Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal

>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > When reading or editing calls to usage_with_options_internal, it is
> > difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
> > (NB there is never a "1, 1" case).
> >
> > Give the flags readable names to improve call-sites.
>
> It is a good idea to explicitly say that this step introduces no
> change in behaviour, and only changes the way how these 0/1 are
> spelled.

>
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > ---
> >  parse-options.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/parse-options.c b/parse-options.c
> > index 5224203ffe..c3222cc9bb 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)
> >       free(options);
> >  }
> >
> > +enum usage_style {
> > +     style_normal = 0,
> > +     style_full = 1,
> > +};
> > +
> > +enum usage_output {
> > +     to_out = 0,
> > +     to_err = 1,
> > +};
>
> These are very much internal implementation detail, so I am not sure
> if this churn is a good thing, though.
>
> For example, it ought to be sufficient, for the purpose of improved
> readability, to instead doing this
>
> >  static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
> >                                                        const char * const *,
> >                                                        const struct option *,
> > -                                                      int, int);
> > +                                                      enum usage_style,
> > +                                                      enum usage_output);
>
> just do
>
>                 int full_usage,
>                 int usage_to_stderr);
>
> here.  Dropping the parameter names in the function prototype is
> allowed, and we encourage to do so in our codebase but _only_ when
> the meaning of each parameter is obvious from their type.  The first
> 3 parameters we see above are of distinct types and except for the
> second one being the usage string given to the users, they should be
> obvious.  But the last two unnamed integers are not obvious and they
> should have been spelled out---otherwise a developer who is adding
> a new callsite cannot work from the prototype alone and has to go to
> the implementation to figure out what to pass.

> > @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
> >               }
> >
> >               if (internal_help && !strcmp(arg + 2, "help-all"))
> > -                     return usage_with_options_internal(ctx, usagestr, options, 1, 0);
> > +                     return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);
>
> But this is not an improvement as-is.  Wrap long lines or the result
> is even harder to read.