| Message ID | 20211026013452.1372122-2-eschwartz@archlinux.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add some more options to the pretty-formats | expand |
On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> 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 <eschwartz@archlinux.org> > --- > 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? > + 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.
On 10/26/21 1:18 AM, Eric Sunshine wrote: > On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> 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 <eschwartz@archlinux.org> >> --- >> 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.
diff --git a/pretty.c b/pretty.c index 73b5ead509..f8b254d2ff 100644 --- 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) { - const char *options[] = { "match", "exclude" }; + struct { + char *name; + enum { OPT_STRING } type; + } option[] = { + { "exclude", OPT_STRING }, + { "match", OPT_STRING }, + }; const char *arg = start; for (;;) { - const char *matched = NULL; + int found = 0; const char *argval; size_t arglen = 0; int i; - for (i = 0; i < ARRAY_SIZE(options); i++) { - if (match_placeholder_arg_value(arg, options[i], &arg, - &argval, &arglen)) { - matched = options[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; + strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval); + found = 1; + } break; } } - if (!matched) + if (!found) break; - if (!arglen) - return 0; - strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval); } return arg - start; }
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 <eschwartz@archlinux.org> --- pretty.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)