From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id E78AD431FBC for ; Wed, 17 Oct 2012 00:48:22 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0n5Fn-4p70ac for ; Wed, 17 Oct 2012 00:48:22 -0700 (PDT) Received: from mail-qc0-f181.google.com (mail-qc0-f181.google.com [209.85.216.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 44366431FB6 for ; Wed, 17 Oct 2012 00:48:22 -0700 (PDT) Received: by mail-qc0-f181.google.com with SMTP id x40so6567779qcp.26 for ; Wed, 17 Oct 2012 00:48:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:subject:in-reply-to:references:user-agent:date:message-id :mime-version:content-type:x-gm-message-state; bh=rcMSW4IVgQikEb1FO4OYRTnQESF27tEKIpBqneVGwSs=; b=DGSBMffJuxvkQ/ym4AhcKMwEDsBRn/Vyltfy4iPIR5UVOL0jrmw8o9wQS4ujBttgnG tufFc/BWfgf4+rH27xWShf23T8P/kZDo0WwB06eRkZ+E2MPT0vmaPdl0OcsM8fvOC8Er zNDm8wdwvsbizf/MFzdTkHOeSBsmJDLSXs3nwo3YqLZ9YVHjATXjYlkwJAcBEr4fk0as oQVJjTKV9r7PEZhN7YHV0RLU0AfEIQnk1TuZQKWv411CjERSWj03Fr/BNNAY1TmMtXKM 56M8+VxNFJcOOdAhmlzF/oM1m+t6cNRHIdJQb6gHwZr2hjqhe3URMWjlP8BdCT40F1Zp E+yg== Received: by 10.49.104.194 with SMTP id gg2mr40693359qeb.6.1350460100482; Wed, 17 Oct 2012 00:48:20 -0700 (PDT) Received: from localhost ([2001:4b98:dc0:43:216:3eff:fe1b:25f3]) by mx.google.com with ESMTPS id z9sm19122876qeg.9.2012.10.17.00.48.18 (version=SSLv3 cipher=OTHER); Wed, 17 Oct 2012 00:48:19 -0700 (PDT) From: Jani Nikula To: Ethan Glasser-Camp , notmuch@notmuchmail.org Subject: Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to notmuch In-Reply-To: <87hapw9x99.fsf@betacantrips.com> References: <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org> <87hapw9x99.fsf@betacantrips.com> User-Agent: Notmuch/0.14+39~ge21970d (http://notmuchmail.org) Emacs/23.2.1 (x86_64-pc-linux-gnu) Date: Wed, 17 Oct 2012 09:48:16 +0200 Message-ID: <87zk3lzghr.fsf@nikula.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Gm-Message-State: ALoCoQlJ+XpqkOUcxHVoTc84rNg1HROel7p7i8cVbuHiF0cxJMs1+alvTXsKoYyiGc69v6WJcNWs X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Oct 2012 07:48:23 -0000 On Mon, 15 Oct 2012, Ethan Glasser-Camp wrote: >> +/* Parse a previously postponed number if one exists. */ >> +static int parse_postponed_number (struct state *state, int v, int n, char d); >> +static int >> +handle_postponed_number (struct state *state, enum field next_field) >> +{ >> + int v = state->postponed_value; >> + int n = state->postponed_length; >> + char d = state->postponed_delim; >> + int r; >> + >> + if (!n) >> + return 0; >> + >> + state->postponed_value = 0; >> + state->postponed_length = 0; >> + state->postponed_delim = 0; > > This could be refactored to be a call to get_postponed_number. Also, > I'd prefer parse_postponed_number be up here, closer to its sole > caller (handle_postponed_number). I decided to nuke the intermediate handle_postponed_number altogether, and fix parse_postponed_number to call get_postponed_number. Thanks for pointing this out. >> +/* >> + * Postpone a number to be handled later. If one exists already, >> + * handle it first. n may be -1 to indicate a keyword that has no >> + * number length. >> + */ >> +static int >> +set_postponed_number (struct state *state, int v, int n) >> +{ >> + int r; >> + char d = state->delim; >> + >> + /* Parse a previously postponed number, if any. */ >> + r = handle_postponed_number (state, TM_NONE); >> + if (r) >> + return r; > > I would love a comment explaining under what circumstances this could > occur and what the caller is expected to do. Any errors anywhere the caller is expected to pop up all the way to the main entry point. I did not verify, but, for example, I'd expect a sequence of "2012 2012 2012" to fail right here. >> +/* >> + * Accepted keywords. >> + */ >> +static struct keyword keywords[] = { >> + /* Weekdays. */ >> + { N_("sun|day"), TM_ABS_WDAY, 0, NULL }, >> + { N_("mon|day"), TM_ABS_WDAY, 1, NULL }, > > Maybe it's just my history with Python, but I'd prefer keywords, which > is a global and a constant, to be written in all caps (KEYWORDS). It's just your history with Python. ;) IMO it's more in line with notmuch coding style as it is. It could be made const though. >> +/* >> + * Compare strings s and keyword. Return number of matching chars on >> + * match, 0 for no match. Match must be at least n chars, or all of >> + * keyword if n < 0, otherwise it's not a match. Use match_case for >> + * case sensitive matching. >> + */ >> +static size_t >> +stringcmp (const char *s, const char *keyword, ssize_t n, bool match_case) >> +{ > > The name of this function makes it look uncomfortably like strcmp(3), > which has a very different calling semantics (specifically the -1, 0, 1 > return value). I'd prefer a name like string_match_keyword. Agreed. BR, Jani.