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.