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 223FB431FAF for ; Sun, 14 Oct 2012 21:26:21 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.799 X-Spam-Level: X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, 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 aU-0FF5mapRf for ; Sun, 14 Oct 2012 21:26:20 -0700 (PDT) Received: from mail-qa0-f46.google.com (mail-qa0-f46.google.com [209.85.216.46]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 59B43431FAE for ; Sun, 14 Oct 2012 21:26:20 -0700 (PDT) Received: by mail-qa0-f46.google.com with SMTP id c26so1204056qad.5 for ; Sun, 14 Oct 2012 21:26:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:subject:in-reply-to:references:user-agent:date:message-id :mime-version:content-type; bh=2a3vNdMJL60KQpOzs9Dh6udgH7wlYnZ6ooqsZPEejh4=; b=xaeNKxXvncPITuZlRVcTCuOktPacgQUE1cv55CEeR9UN5zUuaqXaJx8kp+0s3fACLc f4kqABE075UsZ0qYIxdxCJ/tOm9g7OjDr1Re8lGyAWO64mEC3dcfyBEXKlUiQFX8QrtY xioZknDS9dpBhJ15jzBwa+1aGWE5+toOSNfZEovDNVikljNTlId/Eoc/1q6FnMpJbJq5 H0gMJqVwNYukv5V3NXQFf6s/cO4rt8r9lwmfOOgGnj9oKp43XtciP+yXlOtrkvlRWZJt bNlQ5msTrh2ng706SPHUsXLz9tEP1yK0p1j4VuC3AEDSKBvvrONWT1LpC2dPoqXIX/Vd 3CHw== Received: by 10.224.42.80 with SMTP id r16mr10969680qae.90.1350275179543; Sun, 14 Oct 2012 21:26:19 -0700 (PDT) Received: from smtp.gmail.com (p70-80.acedsl.com. [66.114.70.80]) by mx.google.com with ESMTPS id y17sm9949201qaa.6.2012.10.14.21.26.17 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 14 Oct 2012 21:26:18 -0700 (PDT) From: Ethan Glasser-Camp To: Jani Nikula , notmuch@notmuchmail.org Subject: Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to notmuch In-Reply-To: <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org> References: <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org> User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) Date: Mon, 15 Oct 2012 00:26:10 -0400 Message-ID: <87hapw9x99.fsf@betacantrips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Mon, 15 Oct 2012 04:26:21 -0000 Jani Nikula writes: Hi! I commend you for your work and persistence. This represents a lot of work and I think it's good enough to be merged. I would certainly love to see "last" and "ago" supported but this patch series, and this patch especially, is cumbersome enough that I'd really rather it be merged before it gets any worse. If this patch series isn't accepted, I'd suggest making a patch series that makes a "null" date parser, which just takes strings of date timestamps the way notmuch does now: "12345" -> (time_t) 12345. We're going to need a date parser anyhow, so a patch series that sets up the scaffolding -- separate directory, tests -- could be merged without the actual parser. Then you'd have fewer patches to juggle the next time around. I agree with Tomi Ollila that whether the parser is built in bison or in straight C, as this one is, isn't important. The difficult part of parsing dates isn't the translation from text to parse tree; it's dealing with all the semantic difficulty that comes YYMMDD versus DDMMYY and the difference between "last week" and "last Thursday". I have a few minor quibbles that I would be happy to see addressed after this was merged, or not at all. > +/* 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). > +/* > + * 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. > +/* > + * 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). > +/* > + * 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. Ethan