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

[Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>]

Jani Nikula <jani@nikula.org> writes:

Hi! I commend you for your work and persistence. This represents a lot
of work and I think it's good enough to be merged. I would certainly
love to see "last" and "ago" supported but this patch series, and this
patch especially, is cumbersome enough that I'd really rather it be
merged before it gets any worse.

If this patch series isn't accepted, I'd suggest making a patch series
that makes a "null" date parser, which just takes strings of date
timestamps the way notmuch does now: "12345" -> (time_t) 12345. We're
going to need a date parser anyhow, so a patch series that sets up the
scaffolding -- separate directory, tests -- could be merged without the
actual parser. Then you'd have fewer patches to juggle the next time
around.

I agree with Tomi Ollila that whether the parser is built in bison or in
straight C, as this one is, isn't important. The difficult part of
parsing dates isn't the translation from text to parse tree; it's
dealing with all the semantic difficulty that comes YYMMDD versus DDMMYY
and the difference between "last week" and "last Thursday".

I have a few minor quibbles that I would be happy to see addressed after
this was merged, or not at all.

> +/* 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).

> +/*
> + * 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.

> +/*
> + * 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).

> +/*
> + * 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.

Ethan