From: Jani Nikula <jani@nikula.org>
To: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to
	notmuch
In-Reply-To: <87hapw9x99.fsf@betacantrips.com>
References: <cover.1350164594.git.jani@nikula.org>
	<0296be2a3899653549b3f95e1aa1a4a0632e92e7.1350164594.git.jani@nikula.org>
	<87hapw9x99.fsf@betacantrips.com>
User-Agent: Notmuch/0.14+39~ge21970d (http://notmuchmail.org) Emacs/23.2.1
	(x86_64-pc-linux-gnu)
Date: Wed, 17 Oct 2012 09:48:16 +0200
Message-ID: <87zk3lzghr.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Mon, 15 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:
>> +/* Parse a previously postponed number if one exists. */
>> +static int parse_postponed_number (struct state *state, int v, int n, char d);
>> +static int
>> +handle_postponed_number (struct state *state, enum field next_field)
>> +{
>> +    int v = state->postponed_value;
>> +    int n = state->postponed_length;
>> +    char d = state->postponed_delim;
>> +    int r;
>> +
>> +    if (!n)
>> +	return 0;
>> +
>> +    state->postponed_value = 0;
>> +    state->postponed_length = 0;
>> +    state->postponed_delim = 0;
>
> This could be refactored to be a call to get_postponed_number. Also,
> I'd prefer parse_postponed_number be up here, closer to its sole
> caller (handle_postponed_number).

I decided to nuke the intermediate handle_postponed_number altogether,
and fix parse_postponed_number to call get_postponed_number. Thanks for
pointing this out.

>> +/*
>> + * Postpone a number to be handled later. If one exists already,
>> + * handle it first. n may be -1 to indicate a keyword that has no
>> + * number length.
>> + */
>> +static int
>> +set_postponed_number (struct state *state, int v, int n)
>> +{
>> +    int r;
>> +    char d = state->delim;
>> +
>> +    /* Parse a previously postponed number, if any. */
>> +    r = handle_postponed_number (state, TM_NONE);
>> +    if (r)
>> +	return r;
>
> I would love a comment explaining under what circumstances this could
> occur and what the caller is expected to do.

Any errors anywhere the caller is expected to pop up all the way to the
main entry point. I did not verify, but, for example, I'd expect a
sequence of "2012 2012 2012" to fail right here.

>> +/*
>> + * Accepted keywords.
>> + */
>> +static struct keyword keywords[] = {
>> +    /* Weekdays. */
>> +    { N_("sun|day"),	TM_ABS_WDAY,	0,	NULL },
>> +    { N_("mon|day"),	TM_ABS_WDAY,	1,	NULL },
>
> Maybe it's just my history with Python, but I'd prefer keywords, which
> is a global and a constant, to be written in all caps (KEYWORDS).

It's just your history with Python. ;) IMO it's more in line with
notmuch coding style as it is. It could be made const though.

>> +/*
>> + * Compare strings s and keyword. Return number of matching chars on
>> + * match, 0 for no match. Match must be at least n chars, or all of
>> + * keyword if n < 0, otherwise it's not a match. Use match_case for
>> + * case sensitive matching.
>> + */
>> +static size_t
>> +stringcmp (const char *s, const char *keyword, ssize_t n, bool match_case)
>> +{
>
> The name of this function makes it look uncomfortably like strcmp(3),
> which has a very different calling semantics (specifically the -1, 0, 1
> return value). I'd prefer a name like string_match_keyword.

Agreed.


BR,
Jani.