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