Re: [PATCH] describe: fix --no-exact-match

[Jeff King <peff@peff.net>]

On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:

> And sorry for the unused parameter warning.  Just checked; there are
> 170+ of those remaining before we can enable it in developer mode.  :-/
> Seems worthwhile, though, especially not slapping UNUSED blindly on
> them.

I know, I'm working on it. :) There were more than 800 when I started.
I'm hoping to send out the final patches during this 2.43 cycle.

> Oh.  Do we really need all those?  Anyway, if we added at least the
> most common ones, we'd be better off already, I'd expect:
> 
>    $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
>      29 struct diff_options
>      12 int
>       7 struct grep_opt
>       6 struct rebase_options
>       6 struct apply_state
>       5 struct strbuf
>       5 char
>       4 struct note_data
>       3 struct string_list
>       2 struct strvec
> 
> Increasing the size of the struct like that would increase the total
> memory footprint by a few KB at most -- tolerable.

Hmm, I was thinking that "int_value" might not be so bad. But it seems
like a pretty bad layering violation for parse-options.c to know about
"struct apply_state" and so on. That really is what void pointers are
for.

As for size, you should be able to use a union. All of the types inside
the struct are pointers, so they'd all be the same size. Of course then
you lose some of the safety. If the caller assigned to "int_value" that
is distinct from "foo_value", then you can be sure you will get a NULL
and segfault upon accessing foo_value. With a union, you get whatever
type-punning undefined behavior the compiler sees fit. And the point is
making sure the two match.

We really don't care about separate storage, though. We want type
safety. Maybe an option there would be to let the callback function take
the appropriate type, and cast it. I.e., something like:

  /* define a callback taking the real type */
  int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }

  /* when mentioning a callback, include the type, and make sure that
   * the value and the callback both match it */
  #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \
    { ..., \
      value.type = (v), \
      callback = (int (*)(type *, struct option *opt, ...etc...), \
    }
  ...
  OPT_CALLBACK('f', "foo", int, &my_local_int, my_int_opt)

I'm pretty sure that falls afoul of the standard, though, because we
eventually need to call that function, and we'll do so through a
function pointer that doesn't match its declaration.

I'm not sure there's a portable and non-insane way of doing what we want
here. At least at compile-time. At run-time you could record type
information in an enum and check it in the callback. That seems like
a lot of work for little reward, though.

> > Here's what it looks like, for reference.
> >
> > diff --git a/builtin/describe.c b/builtin/describe.c
> > index b28a4a1f82..718b5c3073 100644
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
> >  static int option_parse_exact_match(const struct option *opt, const char *arg,
> >  				    int unset)
> >  {
> > +	int *val = opt->value;
> 
> This line would assign opt->value_int instead...

Right, though you would not even need it. I snuck it in there because it
gives us an implicit cast from the void pointer. Without it, the current
code would have to do:

  *(int *)opt->value = unset ? DEFAULT_CANDIDATES : 0;

which was too ugly (especially the "progress" one which mentions the
value three times). But if you had pre-cast variables, you could do:

  *opt->value.int = unset ? DEFAULT_CANDIDATES : 0;

directly.

-Peff