73/8a0ffe21e5d4c018cf66aa2dcb90d08300d450

   1 Return-Path: <bremner@unb.ca>\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 EE294431FAF\r
   6         for <notmuch@notmuchmail.org>; Sun,  5 Aug 2012 06:09:31 -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\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
  12         autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id D2NgYn+lUMjD for <notmuch@notmuchmail.org>;\r
  16         Sun,  5 Aug 2012 06:09:28 -0700 (PDT)\r
  17 Received: from tesseract.cs.unb.ca (tesseract.cs.unb.ca [131.202.240.238])\r
  18         (using TLSv1 with cipher AES256-SHA (256/256 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id 6388F431FAE\r
  21         for <notmuch@notmuchmail.org>; Sun,  5 Aug 2012 06:09:28 -0700 (PDT)\r
  22 Received: from fctnnbsc30w-156034089108.dhcp-dynamic.fibreop.nb.bellaliant.net\r
  23         ([156.34.89.108] helo=zancas.localnet)\r
  24         by tesseract.cs.unb.ca with esmtpsa\r
  25         (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.72)\r
  26         (envelope-from <bremner@unb.ca>)\r
  27         id 1Sy0aD-0006Nb-UN; Sun, 05 Aug 2012 10:09:26 -0300\r
  28 Received: from bremner by zancas.localnet with local (Exim 4.80)\r
  29         (envelope-from <bremner@unb.ca>)\r
  30         id 1Sy0Zm-0007zC-Ln; Sun, 05 Aug 2012 10:08:58 -0300\r
  31 From: David Bremner <david@tethera.net>\r
  32 To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
  33 Subject: Re: [PATCH v2 2/7] lib: add a date/time parser module\r
  34 In-Reply-To:\r
  35  <133d16fa9b63e4cd91aa2b8816a6ad7285b3bd4c.1344065790.git.jani@nikula.org>\r
  36 References: <cover.1344065790.git.jani@nikula.org>\r
  37         <133d16fa9b63e4cd91aa2b8816a6ad7285b3bd4c.1344065790.git.jani@nikula.org>\r
  38 User-Agent: Notmuch/0.13.2+104~g5ae484c (http://notmuchmail.org) Emacs/24.1.1\r
  39         (x86_64-pc-linux-gnu)\r
  40 Date: Sun, 05 Aug 2012 10:08:58 -0300\r
  41 Message-ID: <877gtdmqol.fsf@zancas.localnet>\r
  42 MIME-Version: 1.0\r
  43 Content-Type: text/plain\r
  44 X-Spam_bar: -\r
  45 X-BeenThere: notmuch@notmuchmail.org\r
  46 X-Mailman-Version: 2.1.13\r
  47 Precedence: list\r
  48 List-Id: "Use and development of the notmuch mail system."\r
  49         <notmuch.notmuchmail.org>\r
  50 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  51         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  52 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  53 List-Post: <mailto:notmuch@notmuchmail.org>\r
  54 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  55 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  56         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  57 X-List-Received-Date: Sun, 05 Aug 2012 13:09:32 -0000\r
  58 \r
  59 Jani Nikula <jani@nikula.org> writes:\r
  60 \r
  61 > +\r
  62 > +static enum field\r
  63 > +abs_to_rel_field (enum field field)\r
  64 > +{\r
  65 > +    assert (field <= TM_ABS_YEAR);\r
  66 > +\r
  67 > +    /* note: depends on the enum ordering */\r
  68 > +    return field + (TM_REL_SEC - TM_ABS_SEC);\r
  69 > +}\r
  70 > +\r
  71 \r
  72 I wonder if this would be slightly nicer of you defined a TM_FIRST_REL\r
  73 or so as a synonym like TM_NONE and TM_SIZE\r
  74 \r
  75 > +/* get zero value for field */\r
  76 > +static int\r
  77 > +field_zero (enum field field)\r
  78 > +{\r
  79 > +    if (field == TM_ABS_MDAY || field == TM_ABS_MON)\r
  80 > +     return 1;\r
  81 > +    else if (field == TM_ABS_YEAR)\r
  82 > +     return 1970;\r
  83 > +    else\r
  84 > +     return 0;\r
  85 > +}\r
  86 \r
  87 what do you think about using the word "epoch" instead of zero here?\r
  88 \r
  89 > +static bool\r
  90 > +get_postponed_number (struct state *state, int *v, int *n, char *d)\r
  91 > +{\r
  92 \r
  93 I found the 1 letter names not quite obvious here.\r
  94 \r
  95 At this point reading the code, I have not trouble understanding each\r
  96 line/function, but I feel like I'm missing the big picture a bit. \r
  97 What is a postponed number?\r
  98 \r
  99 > +    /*\r
 100 > +     * REVISIT: There could be a "next_field" that would be set from\r
 101 > +     * "field" for the duration of the handle_postponed_number() call,\r
 102 > +     * so it has more information to work with.\r
 103 > +     */\r
 104 \r
 105 The notmuch convention seems to be to use XXX: for this. I'm not sure\r
 106 I'd bother changing, especially if we can't decide how to package this.\r
 107 \r
 108 > +/* Time set helper. No input checking. Use UNSET (-1) to leave unset. */\r
 109 > +static int\r
 110 > +set_abs_time (struct state *state, int hour, int min, int sec)\r
 111 > +{\r
 112 > +    int r;\r
 113 > +\r
 114 > +    if (hour != UNSET) {\r
 115 > +     if ((r = set_field (state, TM_ABS_HOUR, hour)))\r
 116 > +         return r;\r
 117 > +    }\r
 118 \r
 119 So for this function and the next, the first match wins? I don't really\r
 120 see the motivation for this, maybe you can explain a bit.\r
 121 \r
 122 \r
 123 > +    /* timezone codes: offset in minutes. FIXME: add more codes. */\r
 124 \r
 125 Did you think about trying to delegate the list of timezones to the\r
 126 system?\r
 127 \r
 128 > + * Compare strings s and keyword. Return number of matching chars on\r
 129 > + * match, 0 for no match. Match must be at least n chars (n == 0 all\r
 130 > + * of keyword), otherwise it's not a match. Use match_case for case\r
 131 > + * sensitive matching.\r
 132 > + */\r
 133 \r
 134 I guess that's fine, and it is internal, but maybe -1 for whole string\r
 135 would be slightly nicer (although I can't imagine what good matching 0\r
 136 length strings is at the moment).\r
 137 \r
 138 > +     /* Minimum match length. */\r
 139 > +     p = strchr (keyword, '|');\r
 140 > +     if (p) {\r
 141 > +         minlen = p - keyword;\r
 142 > +         memmove (p, p + 1, strlen (p + 1) + 1);\r
 143 > +     }\r
 144 \r
 145 Something about that memmove creeps me out, but I trust you that it's\r
 146 correct. Alternatively I guess you could represent keywords as pairs of\r
 147 strings, which is probably more of a pain.\r
 148 \r
 149 \r
 150 > +\r
 151 > +/* Parse a single number. Typically postpone parsing until later. */\r
 152 \r
 153 OK, so I finally start to understand what a postponed number is :)\r
 154 I understand the compiler likes bottom up declarations, but some\r
 155 top down declarations/comments are needed I think.\r
 156 \r
 157 > +static int\r
 158 > +parse_date (struct state *state, char sep,\r
 159 > +         unsigned long v1, unsigned long v2, unsigned long v3,\r
 160 > +         size_t n1, size_t n2, size_t n3)\r
 161 > +{\r
 162 > +    int year = UNSET, mon = UNSET, mday = UNSET;\r
 163 > +\r
 164 > +    assert (is_date_sep (sep));\r
 165 > +\r
 166 > +    switch (sep) {\r
 167 > +    case '/': /* Date: M[M]/D[D][/YY[YY]] or M[M]/YYYY */\r
 168 \r
 169 If I understand correctly, this chooses between American (?) month, day,\r
 170 year ordering and "sensible" day, month, year ordering by delimiter. I\r
 171 never thought about this as a way to tell (I often write D/M/Y), but\r
 172 that could be just me. I agree it's fine as a convention.\r
 173 \r
 174 > +/*\r
 175 > + * Parse delimiter(s). Return < 0 on error, number of parsed chars on\r
 176 > + * success.\r
 177 > + */\r
 178 \r
 179 So 1:-2 will parse as 1-2 ?, i.e. last delimiter wins? Maybe better to\r
 180 say so explicitly.\r
 181 \r
 182 > +/* Combine absolute and relative fields, and round. */\r
 183 > +static int\r
 184 > +create_output (struct state *state, time_t *t_out, const time_t *tnow,\r
 185 > +            int round)\r
 186 > +{\r
 187 \r
 188 It seems like most of non-obvious logic like (when is "wednesday") is\r
 189 encoded here. From a maintenence point of view, it would be nice to be\r
 190 able to seperate out the heuristic stuff from the mechanical, to the\r
 191 degree that it is possible.\r
 192 \r
 193 d\r