--- /dev/null
+Return-Path: <ethan.glasser.camp@gmail.com>\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 223FB431FAF\r
+ for <notmuch@notmuchmail.org>; Sun, 14 Oct 2012 21:26:21 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.799\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+ FREEMAIL_FROM=0.001, 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 aU-0FF5mapRf for <notmuch@notmuchmail.org>;\r
+ Sun, 14 Oct 2012 21:26:20 -0700 (PDT)\r
+Received: from mail-qa0-f46.google.com (mail-qa0-f46.google.com\r
+ [209.85.216.46]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 59B43431FAE\r
+ for <notmuch@notmuchmail.org>; Sun, 14 Oct 2012 21:26:20 -0700 (PDT)\r
+Received: by mail-qa0-f46.google.com with SMTP id c26so1204056qad.5\r
+ for <notmuch@notmuchmail.org>; Sun, 14 Oct 2012 21:26:19 -0700 (PDT)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
+ h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
+ :mime-version:content-type;\r
+ bh=2a3vNdMJL60KQpOzs9Dh6udgH7wlYnZ6ooqsZPEejh4=;\r
+ b=xaeNKxXvncPITuZlRVcTCuOktPacgQUE1cv55CEeR9UN5zUuaqXaJx8kp+0s3fACLc\r
+ f4kqABE075UsZ0qYIxdxCJ/tOm9g7OjDr1Re8lGyAWO64mEC3dcfyBEXKlUiQFX8QrtY\r
+ xioZknDS9dpBhJ15jzBwa+1aGWE5+toOSNfZEovDNVikljNTlId/Eoc/1q6FnMpJbJq5\r
+ H0gMJqVwNYukv5V3NXQFf6s/cO4rt8r9lwmfOOgGnj9oKp43XtciP+yXlOtrkvlRWZJt\r
+ bNlQ5msTrh2ng706SPHUsXLz9tEP1yK0p1j4VuC3AEDSKBvvrONWT1LpC2dPoqXIX/Vd\r
+ 3CHw==\r
+Received: by 10.224.42.80 with SMTP id r16mr10969680qae.90.1350275179543;\r
+ Sun, 14 Oct 2012 21:26:19 -0700 (PDT)\r
+Received: from smtp.gmail.com (p70-80.acedsl.com. [66.114.70.80])\r
+ by mx.google.com with ESMTPS id y17sm9949201qaa.6.2012.10.14.21.26.17\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Sun, 14 Oct 2012 21:26:18 -0700 (PDT)\r
+From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>\r
+To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to\r
+ notmuch\r
+In-Reply-To:\r
+ <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org>\r
+References: <cover.1350164594.git.jani@nikula.org>\r
+ <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org>\r
+User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.3.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Mon, 15 Oct 2012 00:26:10 -0400\r
+Message-ID: <87hapw9x99.fsf@betacantrips.com>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\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: Mon, 15 Oct 2012 04:26:21 -0000\r
+\r
+Jani Nikula <jani@nikula.org> writes:\r
+\r
+Hi! I commend you for your work and persistence. This represents a lot\r
+of work and I think it's good enough to be merged. I would certainly\r
+love to see "last" and "ago" supported but this patch series, and this\r
+patch especially, is cumbersome enough that I'd really rather it be\r
+merged before it gets any worse.\r
+\r
+If this patch series isn't accepted, I'd suggest making a patch series\r
+that makes a "null" date parser, which just takes strings of date\r
+timestamps the way notmuch does now: "12345" -> (time_t) 12345. We're\r
+going to need a date parser anyhow, so a patch series that sets up the\r
+scaffolding -- separate directory, tests -- could be merged without the\r
+actual parser. Then you'd have fewer patches to juggle the next time\r
+around.\r
+\r
+I agree with Tomi Ollila that whether the parser is built in bison or in\r
+straight C, as this one is, isn't important. The difficult part of\r
+parsing dates isn't the translation from text to parse tree; it's\r
+dealing with all the semantic difficulty that comes YYMMDD versus DDMMYY\r
+and the difference between "last week" and "last Thursday".\r
+\r
+I have a few minor quibbles that I would be happy to see addressed after\r
+this was merged, or not at all.\r
+\r
+> +/* Parse a previously postponed number if one exists. */\r
+> +static int parse_postponed_number (struct state *state, int v, int n, char d);\r
+> +static int\r
+> +handle_postponed_number (struct state *state, enum field next_field)\r
+> +{\r
+> + int v = state->postponed_value;\r
+> + int n = state->postponed_length;\r
+> + char d = state->postponed_delim;\r
+> + int r;\r
+> +\r
+> + if (!n)\r
+> + return 0;\r
+> +\r
+> + state->postponed_value = 0;\r
+> + state->postponed_length = 0;\r
+> + state->postponed_delim = 0;\r
+\r
+This could be refactored to be a call to get_postponed_number. Also, I'd\r
+prefer parse_postponed_number be up here, closer to its sole caller (handle_postponed_number).\r
+\r
+> +/*\r
+> + * Postpone a number to be handled later. If one exists already,\r
+> + * handle it first. n may be -1 to indicate a keyword that has no\r
+> + * number length.\r
+> + */\r
+> +static int\r
+> +set_postponed_number (struct state *state, int v, int n)\r
+> +{\r
+> + int r;\r
+> + char d = state->delim;\r
+> +\r
+> + /* Parse a previously postponed number, if any. */\r
+> + r = handle_postponed_number (state, TM_NONE);\r
+> + if (r)\r
+> + return r;\r
+\r
+I would love a comment explaining under what circumstances this could\r
+occur and what the caller is expected to do.\r
+\r
+> +/*\r
+> + * Accepted keywords.\r
+> + */\r
+> +static struct keyword keywords[] = {\r
+> + /* Weekdays. */\r
+> + { N_("sun|day"), TM_ABS_WDAY, 0, NULL },\r
+> + { N_("mon|day"), TM_ABS_WDAY, 1, NULL },\r
+\r
+Maybe it's just my history with Python, but I'd prefer keywords, which\r
+is a global and a constant, to be written in all caps (KEYWORDS).\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, or all of\r
+> + * keyword if n < 0, otherwise it's not a match. Use match_case for\r
+> + * case sensitive matching.\r
+> + */\r
+> +static size_t\r
+> +stringcmp (const char *s, const char *keyword, ssize_t n, bool match_case)\r
+> +{\r
+\r
+The name of this function makes it look uncomfortably like strcmp(3),\r
+which has a very different calling semantics (specifically the -1, 0, 1\r
+return value). I'd prefer a name like string_match_keyword.\r
+\r
+Ethan\r
projects / notmuch-archives.git / commitdiff