--- /dev/null
+Return-Path: <jani@nikula.org>\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 E29A9431FB6\r
+ for <notmuch@notmuchmail.org>; Sun, 5 Aug 2012 14:43:24 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+ tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 sLCmi06MadNQ for <notmuch@notmuchmail.org>;\r
+ Sun, 5 Aug 2012 14:43:23 -0700 (PDT)\r
+Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com\r
+ [209.85.217.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 77BF0431FAE\r
+ for <notmuch@notmuchmail.org>; Sun, 5 Aug 2012 14:43:23 -0700 (PDT)\r
+Received: by lbbgk1 with SMTP id gk1so1357467lbb.26\r
+ for <notmuch@notmuchmail.org>; Sun, 05 Aug 2012 14:43:22 -0700 (PDT)\r
+X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
+ d=google.com; s=20120113;\r
+ h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
+ :mime-version:content-type:x-gm-message-state;\r
+ bh=r6fHY8U3fg5bLh1fkjXmuQpkIxJT9zgLr/x9epW9zkY=;\r
+ b=IvJclUNJkS4j/OUM8lcBQim4U8lHWboA/LFz3XYwuS2KvKIz5/3HlvbgYnhYg+c6aY\r
+ a26JC7AY1RFWXnL9HgIpHqh99tK+yAM0UzUkEo8g79CHhWAkUqr/jp7XdLQBtwf5TpEP\r
+ /4VhQ+TsnwHxbmguyo4EwhwyXfOxdUlbNm6YW/5ZMou1Emaa771yxvu++7x2AKzt8tgy\r
+ zY2b/Ut0k5grBXPUx6fXPiLZtWScr32+Ylsvr9q5D8QaVRM02TDmh29PRRYXEtXEY1gK\r
+ OHtdS466mYaxygDJZHtrBpjRev2elzU+VBZNy+/4SA6l1ghl8KzWv5KU2TqQOeVORJMr\r
+ C05A==\r
+Received: by 10.112.44.163 with SMTP id f3mr3666046lbm.59.1344203001916;\r
+ Sun, 05 Aug 2012 14:43:21 -0700 (PDT)\r
+Received: from localhost (dsl-hkibrasgw4-fe51df00-27.dhcp.inet.fi.\r
+ [80.223.81.27])\r
+ by mx.google.com with ESMTPS id o5sm3345879lbg.5.2012.08.05.14.43.19\r
+ (version=SSLv3 cipher=OTHER); Sun, 05 Aug 2012 14:43:20 -0700 (PDT)\r
+From: Jani Nikula <jani@nikula.org>\r
+To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH v2 2/7] lib: add a date/time parser module\r
+In-Reply-To: <877gtdmqol.fsf@zancas.localnet>\r
+References: <cover.1344065790.git.jani@nikula.org>\r
+ <133d16fa9b63e4cd91aa2b8816a6ad7285b3bd4c.1344065790.git.jani@nikula.org>\r
+ <877gtdmqol.fsf@zancas.localnet>\r
+User-Agent: Notmuch/0.13.2+125~ga3f01dd (http://notmuchmail.org) Emacs/23.3.1\r
+ (i686-pc-linux-gnu)\r
+Date: Mon, 06 Aug 2012 00:43:18 +0300\r
+Message-ID: <87628xnhft.fsf@nikula.org>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+X-Gm-Message-State:\r
+ ALoCoQnUnXPNlAh2patp9PRqcSgkCYXWz9gG03/tZbRRxxqaaDYFO2Pvzdy6IGNh++82ObcTeW8Z\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 21:43:25 -0000\r
+\r
+\r
+Hi David, thanks for the review!\r
+\r
+On Sun, 05 Aug 2012, David Bremner <david@tethera.net> wrote:\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
+Good idea.\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
+As a non-native speaker, I'll just take your word for it if you think it\r
+would be better. :)\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
+True. I think v and n are used fairly consistently throughout the source\r
+file, so I'll have to consider longer names vs. documentation comment\r
+for them.\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
+\r
+Yeah, perhaps the code reads better if you follow from the top level\r
+parse_time_string() down. Which is at the very end. A "big picture"\r
+documentation comment would do no harm.\r
+\r
+> What is a postponed number?\r
+\r
+I see that you grasped that later, but I'll describe anyway. Parsing is\r
+done from left to right, in a greedy fashion (i.e. match the longest\r
+possible known expression). If after that we encounter a number that we\r
+don't know what to do with yet, postpone it until we move on to the next\r
+expression. The parser for that might eat the preceding "postponed"\r
+number (for example "5 August", 5 would be postponed because we don't\r
+know what to do with yet, but then "August" would be the context to make\r
+it day of month). If that is not the case, the number will be parsed by\r
+parse_postponed_number() as a lonely, single number between there. (Yes,\r
+this should be documented better with the "big picture".)\r
+\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
+Okay, can be changed if needed.\r
+\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
+The whole parser tries to be as unambiguous as possible. If the input\r
+leads to a situation in which any absolute time field (see enum field)\r
+is attempted to set twice, it means it appears twice in the input. For\r
+example, "2012-08-06 August", where the month is set twice. By design,\r
+that is not allowed, and set_field() fails, even if it's the same\r
+value. The only semi-exception is having redundant weekday there, for\r
+example "Monday, August 6".\r
+\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
+No. :) I'll look into it.\r
+\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
+Can be changed.\r
+\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
+I didn't bother to double check it now, but I remember thinking it over\r
+very carefully. :) I agree it could use more clarity to be more\r
+obviously correct.\r
+\r
+(Initially the minlen was coded as an int in the table, but the above\r
+allows the localization to decide how long the match must be.)\r
+\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
+I agree more comments would be in order.\r
+\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
+You understand correctly. It's obviously not a reliable way to tell\r
+unless you make it the convention, which I chose to do. Some parsers,\r
+notably the one in git, also look at the values to see if it could be\r
+D/M/Y if M/D/Y is not possible, but I don't like the ambiguity that\r
+introduces.\r
+\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
+Agreed. It just throws out any extra delimiters. Perhaps this should\r
+also be more strict about the allowed delimiters (now anything is\r
+allowed).\r
+\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
+Agreed. Basically this is step 2, turning all information parsed in step\r
+1 (function parse_input()) into a sensible result. Figuring out the\r
+current time is also done here.\r
+\r
+\r
+BR,\r
+Jani.\r
projects / notmuch-archives.git / commitdiff