git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Ramsay Jones <ramsay@ramsayjones.plus.com>,
	GIT Mailing-list <git@vger.kernel.org>
Subject: Re: Incorrect diff-parseopt conversion?
Date: Sat, 25 May 2019 17:37:02 +0700	[thread overview]
Message-ID: <CACsJy8CrTEAHesK3ZpwLJqu4fCvMGcW3YH-AkSYN79Db_KmdtQ@mail.gmail.com> (raw)
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:
> >> 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.

> 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

      reply	other threads:[~2019-05-25 10:39 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 19:56 Incorrect diff-parseopt conversion? Ramsay Jones
2019-05-22  0:11 ` Duy Nguyen
2019-05-22  0:35   ` Ramsay Jones
2019-05-24  9:40     ` Duy Nguyen
2019-05-24 10:08       ` Ævar Arnfjörð Bjarmason
2019-05-25 10:37         ` Duy Nguyen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACsJy8CrTEAHesK3ZpwLJqu4fCvMGcW3YH-AkSYN79Db_KmdtQ@mail.gmail.com \
    --to=pclouds@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=ramsay@ramsayjones.plus.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).