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

From: Jeff King <peff@peff.net>
Date: Aug 10, 2023, 00:41 UTC
Message-ID: <20230810004127.GD795985@coredump.intra.peff.net>
In-Reply-To: <22e5a87a-cd35-9793-5b6f-6eb368fdf40e@web.de>

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.

Show 17 quoted lines

> 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.

Show 13 quoted lines

> > 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