[PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Jeff Hostetler via GitGitGadget]

This patch series fixes three syntax errors that caused compiler errors with
clang 11.0.0 on MacOS. I've included the error/warning messages in the
commit messages. The offending statements did compile successfully under
clang 14.0.0 on MacOS, so I have to assume that this usage is newer than
what clang 11 supports.

I originally sent these changes in my "Trace2 timers and cleanup and some
cleanup" series on Tuesday, but pulled them into a separate series based on
feedback. I'll omit them from the trace2 series in the next version.

Jeff Hostetler (2):
  builtin/merge-file: fix compiler error on MacOS with clang 11.0.0
  builtin/unpack-objects.c: fix compiler error on MacOS with clang
    11.0.0

 builtin/merge-file.c     | 4 ++--
 builtin/unpack-objects.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)


base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1375%2Fjeffhostetler%2Ffix-clang11-warnings-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1375/jeffhostetler/fix-clang11-warnings-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1375
-- 
gitgitgadget

[PATCH 1/2] builtin/merge-file: fix compiler error on MacOS with clang 11.0.0

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Add extra set of braces around zero initialization of two array/structure
variables to resolve compiler errors/warnings from clang 11.0.0 on MacOS.
This is not needed on clang 14.0.

$ uname -a
Darwin jeffhost-mbp.local 19.6.0 Darwin Kernel Version 19.6.0: \
       Mon Apr 18 21:50:40 PDT 2022; \
       root:xnu-6153.141.62~1/RELEASE_X86_64 x86_64
$ clang -v
Apple clang version 11.0.0 (clang-1100.0.33.17)
Target: x86_64-apple-darwin19.6.0
[...]

$ make builtin/merge-file.o
    CC builtin/merge-file.o
builtin/merge-file.c:29:23: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        mmfile_t mmfs[3] = { 0 };
                             ^
                             {}
builtin/merge-file.c:31:20: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        xmparam_t xmp = { 0 };
                          ^
                          {}
2 errors generated.
make: *** [builtin/merge-file.o] Error 1

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/merge-file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index c923bbf2abb..607c3d3f9e1 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -26,9 +26,9 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {
 	const char *names[3] = { 0 };
-	mmfile_t mmfs[3] = { 0 };
+	mmfile_t mmfs[3] = { { 0 } };
 	mmbuffer_t result = { 0 };
-	xmparam_t xmp = { 0 };
+	xmparam_t xmp = { { 0 } };
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
 	struct option options[] = {
-- 
gitgitgadget

[PATCH 2/2] builtin/unpack-objects.c: fix compiler error on MacOS with clang 11.0.0

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Add an extra set of braces around zero initialization of the `zstream`
variable to resolve compiler error/warning from clang 11.0.0 on MacOS.
This is not needed on clang 14.0.

$ make builtin/unpack-objects.o
    CC builtin/unpack-objects.o
builtin/unpack-objects.c:388:26: error: suggest braces around \
		initialization of subobject [-Werror,-Wmissing-braces]
        git_zstream zstream = { 0 };
                                ^
                                {}
1 error generated.
make: *** [builtin/unpack-objects.o] Error 1

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/unpack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 43789b8ef29..4b16f1592ba 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,
 
 static void stream_blob(unsigned long size, unsigned nr)
 {
-	git_zstream zstream = { 0 };
+	git_zstream zstream = { { 0 } };
 	struct input_zstream_data data = { 0 };
 	struct input_stream in_stream = {
 		.read = feed_input_zstream,
-- 
gitgitgadget

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This patch series fixes three syntax errors that caused compiler errors with
> clang 11.0.0 on MacOS. I've included the error/warning messages in the
> commit messages. The offending statements did compile successfully under
> clang 14.0.0 on MacOS, so I have to assume that this usage is newer than
> what clang 11 supports.

Ah, this looked familiar, and the last time we saw the one in
builtin/unpack-objects.c

https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/

It seems that merge-file.c was OK back then but soon after we
"broke" it with 480a0e30 (merge-file: refactor for subsequent memory
leak fix, 2022-07-01).

    $ git grep -n -e '{ *{ *0 *} *}'
    builtin/merge-index.c:15:	char oids[3][GIT_MAX_HEXSZ + 1] = {{0}};
    builtin/merge-index.c:16:	char modes[3][10] = {{0}};
    builtin/worktree.c:321:		struct config_set cs = { { 0 } };
    merge-strategies.c:93:	xmparam_t xmp = {{0}};
    oidset.h:25:#define OIDSET_INIT { { 0 } }
    worktree.c:795:	struct config_set cs = { { 0 } };

I wonder if we want to introduce some named _INIT for these specific
types?  Perhaps with something like

    /* for config_set */
    #define CONFIG_SET_INIT {{0}}
    /* for xmparam_t */
    #define XMPARAM_INIT {{0}}
    /* for mmfile_t */
    #define MMFILE_INIT {0}
    /* for git_zstream */
    #define GIT_ZSTREAM_INIT {{0}}

	Side note: between { { 0 } } and {{0}} forms that appear in
	the existing code, I picked the "unusual spacing" variant,
	i.e. without any space, hoping that it would signal the fact
	that we would prefer if we didn't have to use these to
	please older compilers to the readers.

Unless we plan to soon declare that clang 11 is too old to matter
anymore, that is.

The rule, tentatively until the compilers that need extra levels of
braces die out, can be "if there is <name>_INIT for the type, you
should use it, but otherwise you can do { 0 }", or something like
that, perhaps?

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Eric Sunshine]

On Thu, Oct 6, 2022 at 4:41 PM Junio C Hamano <gitster@pobox.com> wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > This patch series fixes three syntax errors that caused compiler errors with
> > clang 11.0.0 on MacOS. I've included the error/warning messages in the
> > commit messages. The offending statements did compile successfully under
> > clang 14.0.0 on MacOS, so I have to assume that this usage is newer than
> > what clang 11 supports.
>
> Ah, this looked familiar, and the last time we saw the one in
> builtin/unpack-objects.c
>
> https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/
>
> It seems that merge-file.c was OK back then but soon after we
> "broke" it with 480a0e30 (merge-file: refactor for subsequent memory
> leak fix, 2022-07-01).

For completeness, the merge-file.c instance also got reported:
https://lore.kernel.org/git/365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com/

Re: [PATCH 1/2] builtin/merge-file: fix compiler error on MacOS with clang 11.0.0

[René Scharfe]

Am 06.10.22 um 21:43 schrieb Jeff Hostetler via GitGitGadget:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Add extra set of braces around zero initialization of two array/structure
> variables to resolve compiler errors/warnings from clang 11.0.0 on MacOS.
> This is not needed on clang 14.0.

Not sure how the Apple version numbers map to LLVM versions.  I can
reproduce it with Godbolt's Compiler Explorer with clang 8, but not
with clang 9 or higher: https://godbolt.org/z/f7f7s9xxz

Funny that type of the "inner" member seems to affect the warning.

>
> $ uname -a
> Darwin jeffhost-mbp.local 19.6.0 Darwin Kernel Version 19.6.0: \
>        Mon Apr 18 21:50:40 PDT 2022; \
>        root:xnu-6153.141.62~1/RELEASE_X86_64 x86_64
> $ clang -v
> Apple clang version 11.0.0 (clang-1100.0.33.17)
> Target: x86_64-apple-darwin19.6.0
> [...]
>
> $ make builtin/merge-file.o
>     CC builtin/merge-file.o
> builtin/merge-file.c:29:23: error: suggest braces around initialization \
> 			    of subobject [-Werror,-Wmissing-braces]
>         mmfile_t mmfs[3] = { 0 };
>                              ^
>                              {}

{0} is an idiom to zero-initialize any struct, no matter how deeply
nested.  It's valid C and the compiler warning about it is not helpful.
Shouldn't we rather silence it with -Wno-missing-braces?

On the other hand: Uglifying just three initializations in total isn't
that bad.  The issue may reappear, though, because most people use
compilers that don't issue that spurious warning, I imagine.

> builtin/merge-file.c:31:20: error: suggest braces around initialization \
> 			    of subobject [-Werror,-Wmissing-braces]
>         xmparam_t xmp = { 0 };
>                           ^
>                           {}
> 2 errors generated.
> make: *** [builtin/merge-file.o] Error 1
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  builtin/merge-file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> index c923bbf2abb..607c3d3f9e1 100644
> --- a/builtin/merge-file.c
> +++ b/builtin/merge-file.c
> @@ -26,9 +26,9 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
>  int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  {
>  	const char *names[3] = { 0 };
> -	mmfile_t mmfs[3] = { 0 };
> +	mmfile_t mmfs[3] = { { 0 } };
>  	mmbuffer_t result = { 0 };
> -	xmparam_t xmp = { 0 };
> +	xmparam_t xmp = { { 0 } };
>  	int ret = 0, i = 0, to_stdout = 0;
>  	int quiet = 0;
>  	struct option options[] = {

Re: [PATCH 1/2] builtin/merge-file: fix compiler error on MacOS with clang 11.0.0

[Eric Sunshine]

On Thu, Oct 6, 2022 at 5:25 PM René Scharfe <l.s.r@web.de> wrote:
> Am 06.10.22 um 21:43 schrieb Jeff Hostetler via GitGitGadget:
> > Add extra set of braces around zero initialization of two array/structure
> > variables to resolve compiler errors/warnings from clang 11.0.0 on MacOS.
> > This is not needed on clang 14.0.
>
> Not sure how the Apple version numbers map to LLVM versions.  I can
> reproduce it with Godbolt's Compiler Explorer with clang 8, but not
> with clang 9 or higher: https://godbolt.org/z/f7f7s9xxz
>
> > builtin/merge-file.c:29:23: error: suggest braces around initialization \
> >                           of subobject [-Werror,-Wmissing-braces]
> >         mmfile_t mmfs[3] = { 0 };
> >                              ^
> >                              {}
>
> {0} is an idiom to zero-initialize any struct, no matter how deeply
> nested.  It's valid C and the compiler warning about it is not helpful.
> Shouldn't we rather silence it with -Wno-missing-braces?

That was indeed Ævar's suggestion in [1]:

    Since this is only a warning, and only a practical issue with
    -Werror I wonder if a config.mak.dev change wouldn't be better,
    i.e. to provide a -Wno-missing-braces for this older clang
    version.

The problem is that Apple seems to invent their version numbers out of
thin air with no relation to reality[2,3], so it may take some effort
to work out suitable "version mapping rules" for config.mak.dev, and
nobody has done it yet.

[1]: https://lore.kernel.org/git/220712.86lesy6cri.gmgdl@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/Ys0hhYjPExuNWynE@coredump.intra.peff.net/
[3]: https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/

Re: [PATCH 1/2] builtin/merge-file: fix compiler error on MacOS with clang 11.0.0

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> Shouldn't we rather silence it with -Wno-missing-braces?
>
> That was indeed Ævar's suggestion in [1]:
>
>     Since this is only a warning, and only a practical issue with
>     -Werror I wonder if a config.mak.dev change wouldn't be better,
>     i.e. to provide a -Wno-missing-braces for this older clang
>     version.
>
> The problem is that Apple seems to invent their version numbers out of
> thin air with no relation to reality[2,3], so it may take some effort
> to work out suitable "version mapping rules" for config.mak.dev, and
> nobody has done it yet.
>
> [1]: https://lore.kernel.org/git/220712.86lesy6cri.gmgdl@evledraar.gmail.com/
> [2]: https://lore.kernel.org/git/Ys0hhYjPExuNWynE@coredump.intra.peff.net/
> [3]: https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/

I agree that telling affected users to squelch the warning is a good
first step, and automating it, if there is an easy implementation to
do so, would be a good thing to have on top.

Thanks for digging up the thread.

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 06 2022, Jeff Hostetler via GitGitGadget wrote:

> This patch series fixes three syntax errors that caused compiler errors with
> clang 11.0.0 on MacOS. I've included the error/warning messages in the
> commit messages. The offending statements did compile successfully under
> clang 14.0.0 on MacOS, so I have to assume that this usage is newer than
> what clang 11 supports.
>
> I originally sent these changes in my "Trace2 timers and cleanup and some
> cleanup" series on Tuesday, but pulled them into a separate series based on
> feedback. I'll omit them from the trace2 series in the next version.

The expanded commit messages really help, thanks :)

So, to summarize, these don't fix compiler errors, but warnings, but of
course they're errors with DEVELOPER=1.

We already squash these for an older GCC, per the discussion at [1]. I
think we should just replace this series with something like (untested
on OSX, but it's just copy/pasting a template above it).
	
	diff --git a/config.mak.dev b/config.mak.dev
	index 4fa19d361b7..9b7bccd154c 100644
	--- a/config.mak.dev
	+++ b/config.mak.dev
	@@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
	 endif
	 endif
	 
	+ifeq ($(uname_S),Darwin)
	+ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
	+ifeq ($(filter clang11,$(COMPILER_FEATURES)),)
	+DEVELOPER_CFLAGS += -Wno-missing-braces
	+endif
	+endif
	+endif
	+
	 # https://bugzilla.redhat.com/show_bug.cgi?id=2075786
	 ifneq ($(filter gcc12,$(COMPILER_FEATURES)),)
	 DEVELOPER_CFLAGS += -Wno-error=stringop-overread
	
Or, we can just say that for a <= clang v13 we'll use
-Wno-missing-braces, per:

 * The comment from René at
   http://lore.kernel.org/git/36cd156b-edb2-062c-9422-bf39aad39a6d@web.de
   that older vanilla clang is affected.

 * You having tested Apple clang v14, but not clang v12..v13.

I.e. to emit the whole uname_S bit.

I think it's not important that we try really hard to opt a given
compiler into some maximum set of warnings, we generally want to catch
most things here. As long as some compiler (particularly if it's in CI)
still covers these we should be good.

1. https://lore.kernel.org/git/220712.864jzm65mk.gmgdl@evledraar.gmail.com/

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Jeff Hostetler]

On 10/6/22 3:43 PM, Jeff Hostetler via GitGitGadget wrote:
> This patch series fixes three syntax errors that caused compiler errors with
> clang 11.0.0 on MacOS. I've included the error/warning messages in the
> commit messages. The offending statements did compile successfully under
> clang 14.0.0 on MacOS, so I have to assume that this usage is newer than
> what clang 11 supports.
[...]

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.

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

* 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 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.
   * Clang 11 (from what I can tell (without looking too hard))
     comes with the version of XCode that runs on MacOS 10.15
     Catalina.  (In contrast, clang 14 comes with the version of
     XCode that runs on MacOS 12.6 Monterey.)
   * I can't comment on other old-ish compilers on other platforms.

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

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

So as I said earlier, I'm not sure how to proceed here.

Jeff


[1] 
https://lore.kernel.org/git/pull.1375.git.1665085395.gitgitgadget@gmail.com/

[2] 
https://lore.kernel.org/git/365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com/

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Junio C Hamano]

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/

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[René Scharfe]

Am 07.10.22 um 19:49 schrieb Junio C Hamano:
>
> 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/

Wikipedia has a map that says Apple calls the LLVM clang 8 (i.e. the
real one) "11.0.0" and clang 9 "11.0.3":

https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)_2

Perhaps like this?  (No sign-off because I'm not comfortable with that
make function syntax, but feel free to steal salvageable parts.)

diff --git a/config.mak.dev b/config.mak.dev
index 4fa19d361b..4d59c9044f 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
 endif
 endif

+# LLVM clang older than 9 and Apple clang older than 12 complain
+# about initializing a struct-within-a-struct using just "{ 0 }"
+ifneq ($(filter clang1,$(COMPILER_FEATURES)),)
+ifeq ($(filter $(if $(filter Darwin,$(uname_S)),clang12,clang9),$(COMPILER_FEATURES)),)
+DEVELOPER_CFLAGS += -Wno-missing-braces
+endif
+endif
+
 # https://bugzilla.redhat.com/show_bug.cgi?id=2075786
 ifneq ($(filter gcc12,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-error=stringop-overread

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Jeff Hostetler]

On 10/7/22 4:28 PM, René Scharfe wrote:
> Am 07.10.22 um 19:49 schrieb Junio C Hamano:
>>
>> 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/
> 
> Wikipedia has a map that says Apple calls the LLVM clang 8 (i.e. the
> real one) "11.0.0" and clang 9 "11.0.3":
> 
> https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)_2
> 
> Perhaps like this?  (No sign-off because I'm not comfortable with that
> make function syntax, but feel free to steal salvageable parts.)
> 
> diff --git a/config.mak.dev b/config.mak.dev
> index 4fa19d361b..4d59c9044f 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
>   endif
>   endif
> 
> +# LLVM clang older than 9 and Apple clang older than 12 complain
> +# about initializing a struct-within-a-struct using just "{ 0 }"
> +ifneq ($(filter clang1,$(COMPILER_FEATURES)),)
> +ifeq ($(filter $(if $(filter Darwin,$(uname_S)),clang12,clang9),$(COMPILER_FEATURES)),)
> +DEVELOPER_CFLAGS += -Wno-missing-braces
> +endif
> +endif
> +

So if I understand you correctly, Apple clang 11 is broken
and Apple clang 12 is good.

I was getting ready to send (as soon as the CI finished)
the following a simple to add the -Wno... for clang 11 and
below on Darwin.

+ifeq ($(uname_S),Darwin)
+# Older versions of Apple clang complain about initializing a
+# struct-within-a-struct using just "{0}" rather than "{{0}}".
+# More recent versions do not.  This error is considered a
+# false-positive and not worth fixing, so just disable it.
+ifeq ($(filter clang12,$(COMPILER_FEATURES)),)
+DEVELOPER_CFLAGS += -Wno-missing-braces
+endif
+endif

I'm not sure I understand all of what your suggestion does.

Jeff

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Perhaps like this?  (No sign-off because I'm not comfortable with that
> make function syntax, but feel free to steal salvageable parts.)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 4fa19d361b..4d59c9044f 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
>  endif
>  endif
>
> +# LLVM clang older than 9 and Apple clang older than 12 complain
> +# about initializing a struct-within-a-struct using just "{ 0 }"
> +ifneq ($(filter clang1,$(COMPILER_FEATURES)),)

If it is any version of clang, detect-compiler script should give us
at last "clang1" in $(COMPILER_FEATURES), and filtering the latter
with "clang1" should become "clang1" which should be neq.  We fail
the ifneq if the compiler is not llvm and do not come into this part.

> +ifeq ($(filter $(if $(filter Darwin,$(uname_S)),clang12,clang9),$(COMPILER_FEATURES)),)

On Darwin, $(uname_S) is Darwin and filtering the $(uname_S) with
Darwin will leave Darwin as the "condition" parameter to $(if).
So $(if) evaluates to clang12 on Darwin and clang9 everywhere else.
If $(COMPILER_FEATURES) lacks that cut-off version, that means we
are using clang older than 12 (on macOS) or 9 (everywhere else) and
we come inside this block ...

> +DEVELOPER_CFLAGS += -Wno-missing-braces

... which adds this workaround.

> +endif
> +endif

OK. That is dense, but sounds correct.

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> So if I understand you correctly, Apple clang 11 is broken
> and Apple clang 12 is good.
>
> I was getting ready to send (as soon as the CI finished)
> the following a simple to add the -Wno... for clang 11 and
> below on Darwin.
>
> +ifeq ($(uname_S),Darwin)
> +# Older versions of Apple clang complain about initializing a
> +# struct-within-a-struct using just "{0}" rather than "{{0}}".
> +# More recent versions do not.  This error is considered a
> +# false-positive and not worth fixing, so just disable it.
> +ifeq ($(filter clang12,$(COMPILER_FEATURES)),)
> +DEVELOPER_CFLAGS += -Wno-missing-braces
> +endif
> +endif
>
> I'm not sure I understand all of what your suggestion does.

I do agree that one is dense, but aims for the same thing, and a bit
more.  It might be easier to read if written in longhand, but ...

ifeq ($(uname_s),Darwin)
ifeq ($(filter clang12,$(COMPILER_FEATURES)),)
DEVELOPER_CFLAGS += -Wno-missing-braces
endif
else
ifeq ($(filter clang9,$(COMPILER_FEATURES)),)
DEVELOPER_CFLAGS += -Wno-missing-braces
endif
endif

... we'd need to repeat ourselves, so...

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Eric Sunshine]

On Fri, Oct 7, 2022 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> I do agree that one is dense, but aims for the same thing, and a bit
> more.  It might be easier to read if written in longhand, but ...
>
> ifeq ($(uname_s),Darwin)
> ifeq ($(filter clang12,$(COMPILER_FEATURES)),)
> DEVELOPER_CFLAGS += -Wno-missing-braces
> endif
> else
> ifeq ($(filter clang9,$(COMPILER_FEATURES)),)
> DEVELOPER_CFLAGS += -Wno-missing-braces
> endif
> endif
>
> ... we'd need to repeat ourselves, so...

The repetition is a very minor downside. The big benefit of this
version is that it's easy to understand at-a-glance, unlike the
"dense" version with its high cognitive load.

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[René Scharfe]

Am 08.10.22 um 05:46 schrieb Eric Sunshine:
> On Fri, Oct 7, 2022 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>> I do agree that one is dense, but aims for the same thing, and a bit
>> more.  It might be easier to read if written in longhand, but ...
>>
>> ifeq ($(uname_s),Darwin)
>> ifeq ($(filter clang12,$(COMPILER_FEATURES)),)
>> DEVELOPER_CFLAGS += -Wno-missing-braces
>> endif
>> else
>> ifeq ($(filter clang9,$(COMPILER_FEATURES)),)
>> DEVELOPER_CFLAGS += -Wno-missing-braces
>> endif
>> endif
>>
>> ... we'd need to repeat ourselves, so...
>
> The repetition is a very minor downside. The big benefit of this
> version is that it's easy to understand at-a-glance, unlike the
> "dense" version with its high cognitive load.

It's certainly easier.

It triggers for any compiler that is not clang, though, which is
a bit much.  Alternative compilers may not even understand that
flag.  So the whole thing should be wrapped in

   ifneq ($(filter clang1,$(COMPILER_FEATURES)),)
   ...
   endif

or

   ifneq ($(filter clang%,$(COMPILER_FEATURES)),)
   ...
   endif

René

Re: [PATCH 0/2] Fix syntax errors under clang 11.0.0 on MacOS

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 08.10.22 um 05:46 schrieb Eric Sunshine:
>> On Fri, Oct 7, 2022 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>>> I do agree that one is dense, but aims for the same thing, and a bit
>>> more.  It might be easier to read if written in longhand, but ...
>>>
>>> ifeq ($(uname_s),Darwin)
>>> ifeq ($(filter clang12,$(COMPILER_FEATURES)),)
>>> DEVELOPER_CFLAGS += -Wno-missing-braces
>>> endif
>>> else
>>> ifeq ($(filter clang9,$(COMPILER_FEATURES)),)
>>> DEVELOPER_CFLAGS += -Wno-missing-braces
>>> endif
>>> endif
>>>
>>> ... we'd need to repeat ourselves, so...
>>
>> The repetition is a very minor downside. The big benefit of this
>> version is that it's easy to understand at-a-glance, unlike the
>> "dense" version with its high cognitive load.
>
> It's certainly easier.
>
> It triggers for any compiler that is not clang, though, which is
> a bit much.

Yeah, of course you are right.  In my "here is a translation to
human-language" I did explain what "clang1" was doing in your
version, but of course, I forgot all about it when writing the above
variant.

[PATCH v2] config.mak.dev: disable suggest braces error on old clang versions

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Add the "-Wno-missing-braces" option when building with an old version
of clang to suppress the "suggest braces around initialization" error
in developer mode.

For example, using an old version of clang gives the following errors
(when in DEVELOPER=1 mode):

$ make builtin/merge-file.o
    CC builtin/merge-file.o
builtin/merge-file.c:29:23: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        mmfile_t mmfs[3] = { 0 };
                             ^
                             {}
builtin/merge-file.c:31:20: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        xmparam_t xmp = { 0 };
                          ^
                          {}
2 errors generated.

This example compiles without error/warning with updated versions of
clang.  Since this is an obsolete error, use the -Wno-missing-braces
option to silence the warning when using an older compiler.  This
avoids the need to update the code to use "{{0}}" style
initializations.

Upstream clang version 8 has the problem.  It was fixed in version 9.

The version of clang distributed by Apple with XCode has its own
unique set of version numbers.  Apple clang version 11 has the
problem.  It was fixed in version 12.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
    Fix syntax errors under clang 11.0.0 on MacOS
    
    Here is version 2. This version adds the "-Wno-missing-braces" compiler
    flag when we are using an old version of clang -- rather than updating
    the source files to use the "{{0}}" syntax that was expected by older
    versions of clang.
    
    I've expanded the scope to include fixes for Apple's clang 11 and below
    and non-Apple clang 8 and below.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1375%2Fjeffhostetler%2Ffix-clang11-warnings-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1375/jeffhostetler/fix-clang11-warnings-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1375

Range-diff vs v1:

 1:  7cee38788a7 < -:  ----------- builtin/merge-file: fix compiler error on MacOS with clang 11.0.0
 2:  e5009a325f2 < -:  ----------- builtin/unpack-objects.c: fix compiler error on MacOS with clang 11.0.0
 -:  ----------- > 1:  c6708ed9459 config.mak.dev: disable suggest braces error on old clang versions


 config.mak.dev | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/config.mak.dev b/config.mak.dev
index 4fa19d361b7..981304727c5 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -69,6 +69,31 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
 endif
 endif
 
+# Old versions of clang complain about initializaing a
+# struct-within-a-struct using just "{0}" rather than "{{0}}".  This
+# error is considered a false-positive and not worth fixing, because
+# new clang versions do not, so just disable it.
+#
+# The "bug" was fixed in upstream clang 9.
+#
+# Complicating this is that versions of clang released by Apple have
+# their own version numbers (associated with the corresponding version
+# of XCode) unrelated to the official clang version numbers.
+#
+# The bug was fixed in Apple clang 12.
+#
+ifneq ($(filter clang1,$(COMPILER_FEATURES)),)     # if we are using clang
+ifeq ($(uname_S),Darwin)                           # if we are on darwin
+ifeq ($(filter clang12,$(COMPILER_FEATURES)),)     # if version < 12
+DEVELOPER_CFLAGS += -Wno-missing-braces
+endif
+else                                               # not darwin
+ifeq ($(filter clang9,$(COMPILER_FEATURES)),)      # if version < 9
+DEVELOPER_CFLAGS += -Wno-missing-braces
+endif
+endif
+endif
+
 # https://bugzilla.redhat.com/show_bug.cgi?id=2075786
 ifneq ($(filter gcc12,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-error=stringop-overread

base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164
-- 
gitgitgadget

Re: [PATCH v2] config.mak.dev: disable suggest braces error on old clang versions

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     Here is version 2. This version adds the "-Wno-missing-braces" compiler
>     flag when we are using an old version of clang -- rather than updating
>     the source files to use the "{{0}}" syntax that was expected by older
>     versions of clang.
>     
>     I've expanded the scope to include fixes for Apple's clang 11 and below
>     and non-Apple clang 8 and below.
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 4fa19d361b7..981304727c5 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -69,6 +69,31 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
>  endif
>  endif
>  
> +# Old versions of clang complain about initializaing a
> +# struct-within-a-struct using just "{0}" rather than "{{0}}".  This
> +# error is considered a false-positive and not worth fixing, because
> +# new clang versions do not, so just disable it.
> +#
> +# The "bug" was fixed in upstream clang 9.
> +#
> +# Complicating this is that versions of clang released by Apple have
> +# their own version numbers (associated with the corresponding version
> +# of XCode) unrelated to the official clang version numbers.
> +#
> +# The bug was fixed in Apple clang 12.
> +#
> +ifneq ($(filter clang1,$(COMPILER_FEATURES)),)     # if we are using clang
> +ifeq ($(uname_S),Darwin)                           # if we are on darwin
> +ifeq ($(filter clang12,$(COMPILER_FEATURES)),)     # if version < 12
> +DEVELOPER_CFLAGS += -Wno-missing-braces
> +endif
> +else                                               # not darwin
> +ifeq ($(filter clang9,$(COMPILER_FEATURES)),)      # if version < 9
> +DEVELOPER_CFLAGS += -Wno-missing-braces
> +endif
> +endif
> +endif

Looks very straight-forward, albeit a tad verbose, and simple to
follow.  I think this is equivalent to René's original one-liner.

Will queue.  Thanks.

Re: [PATCH v2] config.mak.dev: disable suggest braces error on old clang versions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Looks very straight-forward, albeit a tad verbose, and simple to
> follow.  I think this is equivalent to René's original one-liner.
>
> Will queue.  Thanks.

Funny that during the whole time we were discussing this patch, I
never recalled b53a5f24 (config.mak.dev: squelch -Wno-missing-braces
for older gcc, 2022-07-29), which was a moral equivalent of this
change for older GCC, until I needed to decide the name of the topic
branch to queue this patch.

I had the topic branch jk/struct-zero-init-with-older-gcc for
b53a5f24 still lying around.  I reused a part of its name and queued
it on jh/struct-zero-init-with-older-clang branch.