Re: Incorrect diff-parseopt conversion?

From: Duy Nguyen <pclouds@gmail.com>
Date: May 25, 2019, 10:37 UTC
Message-ID: <CACsJy8CrTEAHesK3ZpwLJqu4fCvMGcW3YH-AkSYN79Db_KmdtQ@mail.gmail.com>
In-Reply-To: <87mujcf9l6.fsf@evledraar.gmail.com>

On Fri, May 24, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 13 quoted lines

> >> OK, I just had a look at the code in parse-options.c.
> >> Hmm, somewhat ugly! :-D
> >
> > Yeah it's a bit hackish. The problem is parse-options (and also config
> > parser) does not allow passing user data to the callback. Changing it
> > would affect all callsites at once. It's just too scary for me to do
> > it and I chose an uglier way but with smaller impact.
>
> As an aside because I've sometimes wanted to pass such arbitrary data to
> the callback.
>
> Do you mean it's painful to change "struct option" because of all the
> macros around it?

No, it's updating all the callback functions to take a new parameter.

Show 6 quoted lines

> Instead of all callsites, wouldn't that just be a patch simliar to
> bf3ff338a2 ("parse-options: stop abusing 'callback' for lowlevel
> callbacks", 2019-01-27)?
>
> I.e. a special callback that'll be called to return a void* to arbitrary
> data, and then we'll call the callback with that.

That allows more gradual conversion, but we'll add a bunch new OPT_ macros. Not exactly nice.

If we do this, I'd rather go with converting all callback functions at once. Though we probably want to avoid going through the same pain again, by passing a single struct instead, e.g.

int some_callback(const struct option *opt, const struct optval *val);

"arg" and "unset" are in this "val", callback data is in it too. And it may help reduce those -Wunused-paramters that Jeff's trying to kill.

> Or, just passing it around in a global variable, since we know the flow
> of when we setup the data, and when parse_options() is called.

This might work fine with parse_options() at top level (e.g. cmd_xx functions) but apply.c, diff.c and other should not really depend on it. And we could almost avoid that with parse_opt_ctx_t.

-- 
Duy