On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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.
