On 10/26/21 1:18 AM, Eric Sunshine wrote: > On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz wrote: >> It contains option arguments only, not options. We would like to add >> option support here too, but to do that we need to distinguish between >> different types of options. >> >> Lay out the groundwork for distinguishing between bools, strings, etc. >> and move the central logic (validating values and pushing new arguments >> to *args) into the successful match, because that will be fairly >> conditional on what type of argument is being parsed. >> >> Signed-off-by: Eli Schwartz >> --- >> diff --git a/pretty.c b/pretty.c >> @@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts, >> static size_t parse_describe_args(const char *start, struct strvec *args) >> { >> + struct { >> + char *name; >> + enum { OPT_STRING } type; >> + } option[] = { >> + { "exclude", OPT_STRING }, >> + { "match", OPT_STRING }, >> + }; >> const char *arg = start; >> >> for (;;) { >> + int found = 0; >> const char *argval; >> size_t arglen = 0; >> int i; >> >> + for (i = 0; !found && i < ARRAY_SIZE(option); i++) { >> + switch(option[i].type) { >> + case OPT_STRING: >> + if (match_placeholder_arg_value(arg, option[i].name, &arg, >> + &argval, &arglen) && arglen) { >> + if (!arglen) >> + return 0; > > I may be missing something obvious, but how will it be possible for: > > if (!arglen) > return 0; > > to trigger if the `if` immediately above it: > > if (... && arglen) { > > has already asserted that `arglen` is not 0? I don't think you are missing anything here, I simply forgot that halfway through I added a second check to the if, and later moved the code from down below. I think returning 0 is correct here, to avoid pointlessly checking the rest of option[]. So I'll (re-)remove the first check. >> + strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval); >> + found = 1; >> + } >> break; >> } >> } >> + if (!found) >> break; > > The use of `found` to break out of a loop from within a `switch` seems > a bit clunky. An alternative would be to `goto` a label... > >> } >> return arg - start; > > ... which could be introduced just before the `return`. Of course, > this is highly subjective, so not necessarily worth changing. Keeping in mind that this for (;;) { .... break; } was there before me :D I just switched the name/type of the variable it checks... IMO changing to goto is not my business to change (at least not in this patch), and given the "common wisdom" is "goto is evil" I'm not strongly inclined to get into the business of rewriting someone else's code for that. It's too subjective for my taste. -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User