Re: [PATCH v2 1/3] pretty.c: rework describe options parsing for better extensibility

[Eli Schwartz <eschwartz@archlinux.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3523 bytes --]

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.

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]