[PATCH v4 2/9] parse-time-string: add a date/time parser to notmuch
Jani Nikula
jani at nikula.org
Wed Oct 17 00:48:16 PDT 2012
On Mon, 15 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp at 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.
More information about the notmuch
mailing list