Re: [PATCH v4 2/9] parse-time-string: add a date/time parser to notmuch

[Jani Nikula <jani@nikula.org>]

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.