--- /dev/null
+Return-Path: <bremner@unb.ca>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id EE294431FAF\r
+ for <notmuch@notmuchmail.org>; Sun, 5 Aug 2012 06:09:31 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: 0\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
+ autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id D2NgYn+lUMjD for <notmuch@notmuchmail.org>;\r
+ Sun, 5 Aug 2012 06:09:28 -0700 (PDT)\r
+Received: from tesseract.cs.unb.ca (tesseract.cs.unb.ca [131.202.240.238])\r
+ (using TLSv1 with cipher AES256-SHA (256/256 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 6388F431FAE\r
+ for <notmuch@notmuchmail.org>; Sun, 5 Aug 2012 06:09:28 -0700 (PDT)\r
+Received: from fctnnbsc30w-156034089108.dhcp-dynamic.fibreop.nb.bellaliant.net\r
+ ([156.34.89.108] helo=zancas.localnet)\r
+ by tesseract.cs.unb.ca with esmtpsa\r
+ (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.72)\r
+ (envelope-from <bremner@unb.ca>)\r
+ id 1Sy0aD-0006Nb-UN; Sun, 05 Aug 2012 10:09:26 -0300\r
+Received: from bremner by zancas.localnet with local (Exim 4.80)\r
+ (envelope-from <bremner@unb.ca>)\r
+ id 1Sy0Zm-0007zC-Ln; Sun, 05 Aug 2012 10:08:58 -0300\r
+From: David Bremner <david@tethera.net>\r
+To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH v2 2/7] lib: add a date/time parser module\r
+In-Reply-To:\r
+ <133d16fa9b63e4cd91aa2b8816a6ad7285b3bd4c.1344065790.git.jani@nikula.org>\r
+References: <cover.1344065790.git.jani@nikula.org>\r
+ <133d16fa9b63e4cd91aa2b8816a6ad7285b3bd4c.1344065790.git.jani@nikula.org>\r
+User-Agent: Notmuch/0.13.2+104~g5ae484c (http://notmuchmail.org) Emacs/24.1.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Sun, 05 Aug 2012 10:08:58 -0300\r
+Message-ID: <877gtdmqol.fsf@zancas.localnet>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain\r
+X-Spam_bar: -\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Sun, 05 Aug 2012 13:09:32 -0000\r
+\r
+Jani Nikula <jani@nikula.org> writes:\r
+\r
+> +\r
+> +static enum field\r
+> +abs_to_rel_field (enum field field)\r
+> +{\r
+> + assert (field <= TM_ABS_YEAR);\r
+> +\r
+> + /* note: depends on the enum ordering */\r
+> + return field + (TM_REL_SEC - TM_ABS_SEC);\r
+> +}\r
+> +\r
+\r
+I wonder if this would be slightly nicer of you defined a TM_FIRST_REL\r
+or so as a synonym like TM_NONE and TM_SIZE\r
+\r
+> +/* get zero value for field */\r
+> +static int\r
+> +field_zero (enum field field)\r
+> +{\r
+> + if (field == TM_ABS_MDAY || field == TM_ABS_MON)\r
+> + return 1;\r
+> + else if (field == TM_ABS_YEAR)\r
+> + return 1970;\r
+> + else\r
+> + return 0;\r
+> +}\r
+\r
+what do you think about using the word "epoch" instead of zero here?\r
+\r
+> +static bool\r
+> +get_postponed_number (struct state *state, int *v, int *n, char *d)\r
+> +{\r
+\r
+I found the 1 letter names not quite obvious here.\r
+\r
+At this point reading the code, I have not trouble understanding each\r
+line/function, but I feel like I'm missing the big picture a bit. \r
+What is a postponed number?\r
+\r
+> + /*\r
+> + * REVISIT: There could be a "next_field" that would be set from\r
+> + * "field" for the duration of the handle_postponed_number() call,\r
+> + * so it has more information to work with.\r
+> + */\r
+\r
+The notmuch convention seems to be to use XXX: for this. I'm not sure\r
+I'd bother changing, especially if we can't decide how to package this.\r
+\r
+> +/* Time set helper. No input checking. Use UNSET (-1) to leave unset. */\r
+> +static int\r
+> +set_abs_time (struct state *state, int hour, int min, int sec)\r
+> +{\r
+> + int r;\r
+> +\r
+> + if (hour != UNSET) {\r
+> + if ((r = set_field (state, TM_ABS_HOUR, hour)))\r
+> + return r;\r
+> + }\r
+\r
+So for this function and the next, the first match wins? I don't really\r
+see the motivation for this, maybe you can explain a bit.\r
+\r
+\r
+> + /* timezone codes: offset in minutes. FIXME: add more codes. */\r
+\r
+Did you think about trying to delegate the list of timezones to the\r
+system?\r
+\r
+> + * Compare strings s and keyword. Return number of matching chars on\r
+> + * match, 0 for no match. Match must be at least n chars (n == 0 all\r
+> + * of keyword), otherwise it's not a match. Use match_case for case\r
+> + * sensitive matching.\r
+> + */\r
+\r
+I guess that's fine, and it is internal, but maybe -1 for whole string\r
+would be slightly nicer (although I can't imagine what good matching 0\r
+length strings is at the moment).\r
+\r
+> + /* Minimum match length. */\r
+> + p = strchr (keyword, '|');\r
+> + if (p) {\r
+> + minlen = p - keyword;\r
+> + memmove (p, p + 1, strlen (p + 1) + 1);\r
+> + }\r
+\r
+Something about that memmove creeps me out, but I trust you that it's\r
+correct. Alternatively I guess you could represent keywords as pairs of\r
+strings, which is probably more of a pain.\r
+\r
+\r
+> +\r
+> +/* Parse a single number. Typically postpone parsing until later. */\r
+\r
+OK, so I finally start to understand what a postponed number is :)\r
+I understand the compiler likes bottom up declarations, but some\r
+top down declarations/comments are needed I think.\r
+\r
+> +static int\r
+> +parse_date (struct state *state, char sep,\r
+> + unsigned long v1, unsigned long v2, unsigned long v3,\r
+> + size_t n1, size_t n2, size_t n3)\r
+> +{\r
+> + int year = UNSET, mon = UNSET, mday = UNSET;\r
+> +\r
+> + assert (is_date_sep (sep));\r
+> +\r
+> + switch (sep) {\r
+> + case '/': /* Date: M[M]/D[D][/YY[YY]] or M[M]/YYYY */\r
+\r
+If I understand correctly, this chooses between American (?) month, day,\r
+year ordering and "sensible" day, month, year ordering by delimiter. I\r
+never thought about this as a way to tell (I often write D/M/Y), but\r
+that could be just me. I agree it's fine as a convention.\r
+\r
+> +/*\r
+> + * Parse delimiter(s). Return < 0 on error, number of parsed chars on\r
+> + * success.\r
+> + */\r
+\r
+So 1:-2 will parse as 1-2 ?, i.e. last delimiter wins? Maybe better to\r
+say so explicitly.\r
+\r
+> +/* Combine absolute and relative fields, and round. */\r
+> +static int\r
+> +create_output (struct state *state, time_t *t_out, const time_t *tnow,\r
+> + int round)\r
+> +{\r
+\r
+It seems like most of non-obvious logic like (when is "wednesday") is\r
+encoded here. From a maintenence point of view, it would be nice to be\r
+able to seperate out the heuristic stuff from the mechanical, to the\r
+degree that it is possible.\r
+\r
+d\r
projects / notmuch-archives.git / commitdiff