git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Kim Gybels <kgybels@infogroep.be>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, Beat Bolli <dev+git@drbeat.li>,
	git@vger.kernel.org
Subject: Re: [PATCH] builtin/config: work around an unsized array forward declaration
Date: Sun, 8 Jul 2018 01:58:25 +0200	[thread overview]
Message-ID: <20180707235825.GA1546@infogroep.be> (raw)
In-Reply-To: <xmqq1scgkw06.fsf@gitster-ct.c.googlers.com>

On (06/07/18 12:24), Junio C Hamano wrote:
> 
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Jul 05, 2018 at 09:50:53PM +0200, Beat Bolli wrote:
> >
> >> > Your patch is obviously correct, but I think here there might be an even
> >> > simpler solution: just bump option_parse_type() below the declaration,
> >> > since it's the only one that needs it. That hunk is bigger, but the
> >> > overall diff is simpler, and we don't need to carry that extra wrapper
> >> > function.
> >> 
> >> That was dscho's first try in the GitHub issue. It doesn't compile
> >> because the OPT_CALLBACK* macros in the builtin_config_options
> >> declaration inserts a pointer to option_parse_type into the array items.
> >> We need at least one forward declaration, and my patch seemed the least
> >> intrusive.
> >
> > Ah, right, so it actually is mutually recursive.  Forward-declaring
> > option_parse_type() would fix it, along with the reordering. I'm
> > ambivalent between the available options, then; we might as well go with
> > what you posted, then, since it's already done. :)
> 
> Among three, forward declaration of the function with reordering
> that nobody has written except for in the brain smells the best, and
> turning an array to a pointer that points at a separate storage looked
> the worst.  I also am OK with what's already posted, too.

I posted the forward declaration of the function on the Git for
Windows issue:
https://github.com/git-for-windows/git/issues/1735#issuecomment-402825623

I would consider it the minimal fix. The already posted solution is
also OK for me.

It's also possible to use "extern" instead of "static" for the array.
It would, however, not be my preferred solution.

-Kim

      reply	other threads:[~2018-07-07 23:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-05 18:34 [PATCH] builtin/config: work around an unsized array forward declaration Beat Bolli
2018-07-05 19:35 ` Taylor Blau
2018-07-05 19:38 ` Jeff King
2018-07-05 19:50   ` Beat Bolli
2018-07-05 20:02     ` Jeff King
2018-07-06 19:24       ` Junio C Hamano
2018-07-07 23:58         ` Kim Gybels [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=20180707235825.GA1546@infogroep.be \
    --to=kgybels@infogroep.be \
    --cc=dev+git@drbeat.li \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /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).