git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff Hostetler <git@jeffhostetler.com>
Cc: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS
Date: Fri, 07 Oct 2022 10:49:50 -0700	[thread overview]
Message-ID: <xmqqpmf3frr5.fsf@gitster.g> (raw)
In-Reply-To: <0f67ca77-f17f-d844-e689-ca9a9bdf7993@jeffhostetler.com> (Jeff Hostetler's message of "Fri, 7 Oct 2022 11:19:19 -0400")

Jeff Hostetler <git@jeffhostetler.com> writes:

> I didn't realize that these variable initialization fixes would
> spawn such a large response here [1].  Nor that it had been already
> discussed in great length in [2] in July.
>
> I'm not sure how to best proceed here.
>
> * I'm not sure these fixes are important enough to warrant the
>   engineering time to hack up the Makefile or config.mak.uname
>   to conditionally turn off -Wno-missing-braces based on some
>   {platform-os-version, gcc/clang-version} combination.

Developers are expected to be able to cope, but if this something a
few easy lines in config.mak.uname can help, we'd better do so,
instead of having dozens of folks on the macOS add the same line to
their config.mak file.

> * While -Wno-missing-braces option may prevent the warning/error
>   (depending on -Werror) for these "{0}" should be "{{0}}"
>   errors, do we know that this won't hide real problems.  (I mean
>   we tend to only see it for these {0} / {{0}} false alarms, but
>   I'd hate to lose the protection for non-false alarms.)

It may find real problems, but folks on other platforms are running
without the check disabled, so we should be OK.  More importantly,
folks with a more recent compilers on macOS are running with the
check, so we should be able to give a reasonable coverage to
platform specific code, too.

> * The suggestion to use a <type>_INIT macro to hide the {0} or
>   {{0}} may help in the:
>         xmparam_t xmp = XMPARAM_INIT;
>   case, but in the `mmfile_t mmfs[3]` case, we have an array of
>   that type, so we'd need something like:
>      mmfile_t mmfs[3] = { MMFILE_INIT }; or
>      mmfile_t mmfs[3] = MMFILE_INIT_ARRAY;
>   for the macros to make sense.
>   I'm not sure either of these two is better than just writing "{{0}}".

I do not like that suggestion at all.  I think it is the right
approach to use -Wno-missing-braces with older and buggy compilers
until they die out.

> * I wasn't sure which compiler versions we *want* to support or
>   want to drop support for.
>   * I've only thought about it in the context of clang on MacOS.

I think that is OK.  IIRC, we have more or less good grasp on the
cut-off version of clang as the upstream calls it, but the problem
is clang shipped with macOS lies and gives a version number more
recent than it actually is.

> * I'm not the first one who has stumbled over this and had to
>   rediscover the solution.  So I'd hate to just kick this down
>   the road some more, but then again I'd hate to waste a lot of
>   time on it -- for very little actual functional value.

Exactly.

> * Is "{{0}}" really that ugly ???

Ugly is not the issue.  Folks on more recent platforms not being
able to tell when to use the extra parentheses is, because their
compilers do not have this bug.

My preference is to flip the -Wno-missing-braces bit in
config.mak.uname only for folks who use the version of clang on
macOS when that clang claims to be clang11 (my understanding of
René's experiment[*] is that versions of (real) clang 9 or newer
perfectly well understand that {0} is an accpetable way to specify
zero initialization for any structure, with possible nesting).

[Reference]

* https://lore.kernel.org/git/36cd156b-edb2-062c-9422-bf39aad39a6d@web.de/

  reply	other threads:[~2022-10-07 17:49 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-06 19:43 [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS Jeff Hostetler via GitGitGadget
2022-10-06 19:43 ` [PATCH 1/2] builtin/merge-file: fix compiler error on MacOS with clang 11.0.0 Jeff Hostetler via GitGitGadget
2022-10-06 21:09   ` René Scharfe
2022-10-06 22:30     ` Eric Sunshine
2022-10-06 22:51       ` Junio C Hamano
2022-10-06 19:43 ` [PATCH 2/2] builtin/unpack-objects.c: " Jeff Hostetler via GitGitGadget
2022-10-06 20:38 ` [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS Junio C Hamano
2022-10-06 20:58   ` Eric Sunshine
2022-10-07 11:02 ` Ævar Arnfjörð Bjarmason
2022-10-07 15:19 ` Jeff Hostetler
2022-10-07 17:49   ` Junio C Hamano [this message]
2022-10-07 20:28     ` René Scharfe
2022-10-07 20:56       ` Jeff Hostetler
2022-10-07 21:33         ` Junio C Hamano
2022-10-08  3:46           ` Eric Sunshine
2022-10-08  6:52             ` René Scharfe
2022-10-09  5:19               ` Junio C Hamano
2022-10-07 21:30       ` Junio C Hamano
2022-10-10 15:39 ` [PATCH v2] config.mak.dev: disable suggest braces error on old clang versions Jeff Hostetler via GitGitGadget
2022-10-10 18:13   ` Junio C Hamano
2022-10-11  0:15     ` Junio C Hamano

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

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

  git send-email \
    --in-reply-to=xmqqpmf3frr5.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.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.
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 NNTP newsgroup(s).