ee/4f0f937fb0b3d3a40daf27eabf3ea6e362de08

   1 Return-Path: <jani@nikula.org>\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 E78AD431FBC\r
   6         for <notmuch@notmuchmail.org>; Wed, 17 Oct 2012 00:48:22 -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.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 0n5Fn-4p70ac for <notmuch@notmuchmail.org>;\r
  16         Wed, 17 Oct 2012 00:48:22 -0700 (PDT)\r
  17 Received: from mail-qc0-f181.google.com (mail-qc0-f181.google.com\r
  18         [209.85.216.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id 44366431FB6\r
  21         for <notmuch@notmuchmail.org>; Wed, 17 Oct 2012 00:48:22 -0700 (PDT)\r
  22 Received: by mail-qc0-f181.google.com with SMTP id x40so6567779qcp.26\r
  23         for <notmuch@notmuchmail.org>; Wed, 17 Oct 2012 00:48:20 -0700 (PDT)\r
  24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
  25         d=google.com; s=20120113;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type:x-gm-message-state;\r
  28         bh=rcMSW4IVgQikEb1FO4OYRTnQESF27tEKIpBqneVGwSs=;\r
  29         b=DGSBMffJuxvkQ/ym4AhcKMwEDsBRn/Vyltfy4iPIR5UVOL0jrmw8o9wQS4ujBttgnG\r
  30         tufFc/BWfgf4+rH27xWShf23T8P/kZDo0WwB06eRkZ+E2MPT0vmaPdl0OcsM8fvOC8Er\r
  31         zNDm8wdwvsbizf/MFzdTkHOeSBsmJDLSXs3nwo3YqLZ9YVHjATXjYlkwJAcBEr4fk0as\r
  32         oQVJjTKV9r7PEZhN7YHV0RLU0AfEIQnk1TuZQKWv411CjERSWj03Fr/BNNAY1TmMtXKM\r
  33         56M8+VxNFJcOOdAhmlzF/oM1m+t6cNRHIdJQb6gHwZr2hjqhe3URMWjlP8BdCT40F1Zp\r
  34         E+yg==\r
  35 Received: by 10.49.104.194 with SMTP id gg2mr40693359qeb.6.1350460100482;\r
  36         Wed, 17 Oct 2012 00:48:20 -0700 (PDT)\r
  37 Received: from localhost ([2001:4b98:dc0:43:216:3eff:fe1b:25f3])\r
  38         by mx.google.com with ESMTPS id z9sm19122876qeg.9.2012.10.17.00.48.18\r
  39         (version=SSLv3 cipher=OTHER); Wed, 17 Oct 2012 00:48:19 -0700 (PDT)\r
  40 From: Jani Nikula <jani@nikula.org>\r
  41 To: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>, notmuch@notmuchmail.org\r
  42 Subject: Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to\r
  43         notmuch\r
  44 In-Reply-To: <87hapw9x99.fsf@betacantrips.com>\r
  45 References: <cover.1350164594.git.jani@nikula.org>\r
  46         <0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org>\r
  47         <87hapw9x99.fsf@betacantrips.com>\r
  48 User-Agent: Notmuch/0.14+39~ge21970d (http://notmuchmail.org) Emacs/23.2.1\r
  49         (x86_64-pc-linux-gnu)\r
  50 Date: Wed, 17 Oct 2012 09:48:16 +0200\r
  51 Message-ID: <87zk3lzghr.fsf@nikula.org>\r
  52 MIME-Version: 1.0\r
  53 Content-Type: text/plain; charset=us-ascii\r
  54 X-Gm-Message-State:\r
  55  ALoCoQlJ+XpqkOUcxHVoTc84rNg1HROel7p7i8cVbuHiF0cxJMs1+alvTXsKoYyiGc69v6WJcNWs\r
  56 X-BeenThere: notmuch@notmuchmail.org\r
  57 X-Mailman-Version: 2.1.13\r
  58 Precedence: list\r
  59 List-Id: "Use and development of the notmuch mail system."\r
  60         <notmuch.notmuchmail.org>\r
  61 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  62         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  63 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  64 List-Post: <mailto:notmuch@notmuchmail.org>\r
  65 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  66 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  67         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  68 X-List-Received-Date: Wed, 17 Oct 2012 07:48:23 -0000\r
  69 \r
  70 On Mon, 15 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:\r
  71 >> +/* Parse a previously postponed number if one exists. */\r
  72 >> +static int parse_postponed_number (struct state *state, int v, int n, char d);\r
  73 >> +static int\r
  74 >> +handle_postponed_number (struct state *state, enum field next_field)\r
  75 >> +{\r
  76 >> +    int v = state->postponed_value;\r
  77 >> +    int n = state->postponed_length;\r
  78 >> +    char d = state->postponed_delim;\r
  79 >> +    int r;\r
  80 >> +\r
  81 >> +    if (!n)\r
  82 >> +    return 0;\r
  83 >> +\r
  84 >> +    state->postponed_value = 0;\r
  85 >> +    state->postponed_length = 0;\r
  86 >> +    state->postponed_delim = 0;\r
  87 >\r
  88 > This could be refactored to be a call to get_postponed_number. Also,\r
  89 > I'd prefer parse_postponed_number be up here, closer to its sole\r
  90 > caller (handle_postponed_number).\r
  91 \r
  92 I decided to nuke the intermediate handle_postponed_number altogether,\r
  93 and fix parse_postponed_number to call get_postponed_number. Thanks for\r
  94 pointing this out.\r
  95 \r
  96 >> +/*\r
  97 >> + * Postpone a number to be handled later. If one exists already,\r
  98 >> + * handle it first. n may be -1 to indicate a keyword that has no\r
  99 >> + * number length.\r
 100 >> + */\r
 101 >> +static int\r
 102 >> +set_postponed_number (struct state *state, int v, int n)\r
 103 >> +{\r
 104 >> +    int r;\r
 105 >> +    char d = state->delim;\r
 106 >> +\r
 107 >> +    /* Parse a previously postponed number, if any. */\r
 108 >> +    r = handle_postponed_number (state, TM_NONE);\r
 109 >> +    if (r)\r
 110 >> +    return r;\r
 111 >\r
 112 > I would love a comment explaining under what circumstances this could\r
 113 > occur and what the caller is expected to do.\r
 114 \r
 115 Any errors anywhere the caller is expected to pop up all the way to the\r
 116 main entry point. I did not verify, but, for example, I'd expect a\r
 117 sequence of "2012 2012 2012" to fail right here.\r
 118 \r
 119 >> +/*\r
 120 >> + * Accepted keywords.\r
 121 >> + */\r
 122 >> +static struct keyword keywords[] = {\r
 123 >> +    /* Weekdays. */\r
 124 >> +    { N_("sun|day"),        TM_ABS_WDAY,    0,      NULL },\r
 125 >> +    { N_("mon|day"),        TM_ABS_WDAY,    1,      NULL },\r
 126 >\r
 127 > Maybe it's just my history with Python, but I'd prefer keywords, which\r
 128 > is a global and a constant, to be written in all caps (KEYWORDS).\r
 129 \r
 130 It's just your history with Python. ;) IMO it's more in line with\r
 131 notmuch coding style as it is. It could be made const though.\r
 132 \r
 133 >> +/*\r
 134 >> + * Compare strings s and keyword. Return number of matching chars on\r
 135 >> + * match, 0 for no match. Match must be at least n chars, or all of\r
 136 >> + * keyword if n < 0, otherwise it's not a match. Use match_case for\r
 137 >> + * case sensitive matching.\r
 138 >> + */\r
 139 >> +static size_t\r
 140 >> +stringcmp (const char *s, const char *keyword, ssize_t n, bool match_case)\r
 141 >> +{\r
 142 >\r
 143 > The name of this function makes it look uncomfortably like strcmp(3),\r
 144 > which has a very different calling semantics (specifically the -1, 0, 1\r
 145 > return value). I'd prefer a name like string_match_keyword.\r
 146 \r
 147 Agreed.\r
 148 \r
 149 \r
 150 BR,\r
 151 Jani.\r