On Tue, 05 Jun 2012, Peter Wang <noval...@gmail.com> wrote: > On Sun, 3 Jun 2012 12:48:48 +0100, Mark Walters <markwalters1...@gmail.com> > wrote: >> >> +static notmuch_bool_t >> +_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char >> *arg_str) { >> + >> + char *endptr; >> + if (next == 0 || arg_str[0] == 0) { >> + fprintf (stderr, "Option \"%s\" needs an integer argument.\n", >> arg_desc->name); >> + return FALSE; >> + } >> + >> + *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10); >> + if (*endptr == 0) >> + return TRUE; > > It's usually clearer to write '\0' for the null character.
Yes I agree: fixed. I also changed the other instances. >> @@ -99,20 +133,13 @@ parse_option (const char *arg, >> char next = arg[strlen (try->name)]; >> const char *value= arg+strlen(try->name)+1; >> >> - char *endptr; >> - >> - /* Everything but boolean arguments (switches) needs a >> - * delimiter, and a non-zero length value. Boolean >> - * arguments may take an optional =true or =false value. >> - */ >> - if (next != '=' && next != ':' && next != 0) return FALSE; >> - if (next == 0) { >> - if (try->opt_type != NOTMUCH_OPT_BOOLEAN && >> - try->opt_type != NOTMUCH_OPT_KEYWORD) >> - return FALSE; >> - } else { >> - if (value[0] == 0) return FALSE; >> - } >> + /* If this is not the end of the argument (i.e. the next >> + * character is not a space or a delimiter) we stop >> + * parsing for this option but allow the parsing to >> + * continue to for other options. This should allow >> + * options to be initial segments of other options. */ > > It took me a little while to figure out what the last sentence was > about. Perhaps: > > If we have not reached the end of the argument (i.e. the next > character is not a space or delimiter) then the argument could > still match a longer option name later in the option table. This is much clearer, thanks! > (otherwise, "continue to for other") > >> + if (next != '=' && next != ':' && next != 0) >> + goto DONE_THIS_OPTION; > > The `goto' could be expressed as a `continue' in a `for' loop, AFAICS. This is also much nicer. Updated patch follows Thanks for the review! Best wishes Mark _______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch