Re: [PATCH v2 1/3] pretty.c: rework describe options parsing for better extensibility
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Oct 26, 2021, 05:18 UTC
- Message-ID
- <CAPig+cTWeN9_Z1jNLyyMsbRS4oOoyrPAWa3+JdCtsgE2B-rKFg@mail.gmail.com>
- In-Reply-To
- <20211026013452.1372122-2-eschwartz@archlinux.org>
On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
Show 37 quoted lines
> 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?
Show 8 quoted lines
> + 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.