00/600b9e24b3970cd5c44b5ddd577dd66d04d410

   1 Return-Path: <ethan.glasser.camp@gmail.com>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 223FB431FAF\r
   6         for <notmuch@notmuchmail.org>; Sun, 14 Oct 2012 21:26:21 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.799\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id aU-0FF5mapRf for <notmuch@notmuchmail.org>;\r
  17         Sun, 14 Oct 2012 21:26:20 -0700 (PDT)\r
  18 Received: from mail-qa0-f46.google.com (mail-qa0-f46.google.com\r
  19         [209.85.216.46]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 59B43431FAE\r
  22         for <notmuch@notmuchmail.org>; Sun, 14 Oct 2012 21:26:20 -0700 (PDT)\r
  23 Received: by mail-qa0-f46.google.com with SMTP id c26so1204056qad.5\r
  24         for <notmuch@notmuchmail.org>; Sun, 14 Oct 2012 21:26:19 -0700 (PDT)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type;\r
  28         bh=2a3vNdMJL60KQpOzs9Dh6udgH7wlYnZ6ooqsZPEejh4=;\r
  29         b=xaeNKxXvncPITuZlRVcTCuOktPacgQUE1cv55CEeR9UN5zUuaqXaJx8kp+0s3fACLc\r
  30         f4kqABE075UsZ0qYIxdxCJ/tOm9g7OjDr1Re8lGyAWO64mEC3dcfyBEXKlUiQFX8QrtY\r
  31         xioZknDS9dpBhJ15jzBwa+1aGWE5+toOSNfZEovDNVikljNTlId/Eoc/1q6FnMpJbJq5\r
  32         H0gMJqVwNYukv5V3NXQFf6s/cO4rt8r9lwmfOOgGnj9oKp43XtciP+yXlOtrkvlRWZJt\r
  33         bNlQ5msTrh2ng706SPHUsXLz9tEP1yK0p1j4VuC3AEDSKBvvrONWT1LpC2dPoqXIX/Vd\r
  34         3CHw==\r
  35 Received: by 10.224.42.80 with SMTP id r16mr10969680qae.90.1350275179543;\r
  36         Sun, 14 Oct 2012 21:26:19 -0700 (PDT)\r
  37 Received: from smtp.gmail.com (p70-80.acedsl.com. [66.114.70.80])\r
  38         by mx.google.com with ESMTPS id y17sm9949201qaa.6.2012.10.14.21.26.17\r
  39         (version=TLSv1/SSLv3 cipher=OTHER);\r
  40         Sun, 14 Oct 2012 21:26:18 -0700 (PDT)\r
  41 From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>\r
  42 To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
  43 Subject: Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to\r
  44         notmuch\r
  45 In-Reply-To:\r
  46  <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org>\r
  47 References: <cover.1350164594.git.jani@nikula.org>\r
  48         <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org>\r
  49 User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.3.1\r
  50         (x86_64-pc-linux-gnu)\r
  51 Date: Mon, 15 Oct 2012 00:26:10 -0400\r
  52 Message-ID: <87hapw9x99.fsf@betacantrips.com>\r
  53 MIME-Version: 1.0\r
  54 Content-Type: text/plain; charset=us-ascii\r
  55 X-BeenThere: notmuch@notmuchmail.org\r
  56 X-Mailman-Version: 2.1.13\r
  57 Precedence: list\r
  58 List-Id: "Use and development of the notmuch mail system."\r
  59         <notmuch.notmuchmail.org>\r
  60 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  61         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  62 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  63 List-Post: <mailto:notmuch@notmuchmail.org>\r
  64 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  65 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  67 X-List-Received-Date: Mon, 15 Oct 2012 04:26:21 -0000\r
  68 \r
  69 Jani Nikula <jani@nikula.org> writes:\r
  70 \r
  71 Hi! I commend you for your work and persistence. This represents a lot\r
  72 of work and I think it's good enough to be merged. I would certainly\r
  73 love to see "last" and "ago" supported but this patch series, and this\r
  74 patch especially, is cumbersome enough that I'd really rather it be\r
  75 merged before it gets any worse.\r
  76 \r
  77 If this patch series isn't accepted, I'd suggest making a patch series\r
  78 that makes a "null" date parser, which just takes strings of date\r
  79 timestamps the way notmuch does now: "12345" -> (time_t) 12345. We're\r
  80 going to need a date parser anyhow, so a patch series that sets up the\r
  81 scaffolding -- separate directory, tests -- could be merged without the\r
  82 actual parser. Then you'd have fewer patches to juggle the next time\r
  83 around.\r
  84 \r
  85 I agree with Tomi Ollila that whether the parser is built in bison or in\r
  86 straight C, as this one is, isn't important. The difficult part of\r
  87 parsing dates isn't the translation from text to parse tree; it's\r
  88 dealing with all the semantic difficulty that comes YYMMDD versus DDMMYY\r
  89 and the difference between "last week" and "last Thursday".\r
  90 \r
  91 I have a few minor quibbles that I would be happy to see addressed after\r
  92 this was merged, or not at all.\r
  93 \r
  94 > +/* Parse a previously postponed number if one exists. */\r
  95 > +static int parse_postponed_number (struct state *state, int v, int n, char d);\r
  96 > +static int\r
  97 > +handle_postponed_number (struct state *state, enum field next_field)\r
  98 > +{\r
  99 > +    int v = state->postponed_value;\r
 100 > +    int n = state->postponed_length;\r
 101 > +    char d = state->postponed_delim;\r
 102 > +    int r;\r
 103 > +\r
 104 > +    if (!n)\r
 105 > +     return 0;\r
 106 > +\r
 107 > +    state->postponed_value = 0;\r
 108 > +    state->postponed_length = 0;\r
 109 > +    state->postponed_delim = 0;\r
 110 \r
 111 This could be refactored to be a call to get_postponed_number. Also, I'd\r
 112 prefer parse_postponed_number be up here, closer to its sole caller (handle_postponed_number).\r
 113 \r
 114 > +/*\r
 115 > + * Postpone a number to be handled later. If one exists already,\r
 116 > + * handle it first. n may be -1 to indicate a keyword that has no\r
 117 > + * number length.\r
 118 > + */\r
 119 > +static int\r
 120 > +set_postponed_number (struct state *state, int v, int n)\r
 121 > +{\r
 122 > +    int r;\r
 123 > +    char d = state->delim;\r
 124 > +\r
 125 > +    /* Parse a previously postponed number, if any. */\r
 126 > +    r = handle_postponed_number (state, TM_NONE);\r
 127 > +    if (r)\r
 128 > +     return r;\r
 129 \r
 130 I would love a comment explaining under what circumstances this could\r
 131 occur and what the caller is expected to do.\r
 132 \r
 133 > +/*\r
 134 > + * Accepted keywords.\r
 135 > + */\r
 136 > +static struct keyword keywords[] = {\r
 137 > +    /* Weekdays. */\r
 138 > +    { N_("sun|day"), TM_ABS_WDAY,    0,      NULL },\r
 139 > +    { N_("mon|day"), TM_ABS_WDAY,    1,      NULL },\r
 140 \r
 141 Maybe it's just my history with Python, but I'd prefer keywords, which\r
 142 is a global and a constant, to be written in all caps (KEYWORDS).\r
 143 \r
 144 > +/*\r
 145 > + * Compare strings s and keyword. Return number of matching chars on\r
 146 > + * match, 0 for no match. Match must be at least n chars, or all of\r
 147 > + * keyword if n < 0, otherwise it's not a match. Use match_case for\r
 148 > + * case sensitive matching.\r
 149 > + */\r
 150 > +static size_t\r
 151 > +stringcmp (const char *s, const char *keyword, ssize_t n, bool match_case)\r
 152 > +{\r
 153 \r
 154 The name of this function makes it look uncomfortably like strcmp(3),\r
 155 which has a very different calling semantics (specifically the -1, 0, 1\r
 156 return value). I'd prefer a name like string_match_keyword.\r
 157 \r
 158 Ethan\r