[PATCH] unpack-objects: fix compilation warning/error due to missing braces

[PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Eric Sunshine]

On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing
braces around initialization of a subobject, which is problematic when
building with `DEVELOPER=YesPlease` which enables `-Werror`:

    builtin/unpack-objects.c:388:26: error: suggest braces around
        initialization of subobject [-Werror,-Wmissing-braces]
            git_zstream zstream = { 0 };

[1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)"

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Notes:
    This is atop 'hx/unpack-streaming' which is already in 'next'.
    All the CI builds are fine with this change.

    As I understand it, this should be a safe change; the fields which
    follow `z_stream z` in `git_zstream` will be initialized to zero
    since the first field has an explicit initializer.

 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 43789b8ef2..c606c92e37 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,
-- 
2.37.0.236.gcef32db0b6.dirty

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Han Xin]

On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing
> braces around initialization of a subobject, which is problematic when
> building with `DEVELOPER=YesPlease` which enables `-Werror`:
>
>     builtin/unpack-objects.c:388:26: error: suggest braces around
>         initialization of subobject [-Werror,-Wmissing-braces]
>             git_zstream zstream = { 0 };
>
> [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)"
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> Notes:
>     This is atop 'hx/unpack-streaming' which is already in 'next'.
>     All the CI builds are fine with this change.
>
>     As I understand it, this should be a safe change; the fields which
>     follow `z_stream z` in `git_zstream` will be initialized to zero
>     since the first field has an explicit initializer.
>
>  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 43789b8ef2..c606c92e37 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,
> --
> 2.37.0.236.gcef32db0b6.dirty
>

Not a comment, just wondering, when should I use "{ { 0 } }" and when
should I use "{ 0 }"?

I didn't get the error with "Apple clang version 13.0.0
(clang-1300.0.29.30)",  because it's
a higher version ?

Thanks.
-Han Xin

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Eric Sunshine]

On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:
> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing
> > braces around initialization of a subobject, which is problematic when
> > building with `DEVELOPER=YesPlease` which enables `-Werror`:
> >
> >     builtin/unpack-objects.c:388:26: error: suggest braces around
> >         initialization of subobject [-Werror,-Wmissing-braces]
> >             git_zstream zstream = { 0 };
> >
> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)"
> > -       git_zstream zstream = { 0 };
> > +       git_zstream zstream = {{ 0 }};
>
> Not a comment, just wondering, when should I use "{ { 0 } }" and when
> should I use "{ 0 }"?
>
> I didn't get the error with "Apple clang version 13.0.0
> (clang-1300.0.29.30)",  because it's
> a higher version ?

I don't have a good answer. More modern `clang` versions don't seem to
complain about plain old `{0}` here, but the older `clang` with which
I'm stuck does complain. Aside from actually building the project with
an older `clang` (or older Apple-specific `clang`), it may be
sufficient to inspect the structure that's being initialized to see if
the first element is itself a subobject. However, I'm not sure it's
worth the effort to do so considering how rare this problem seems to
be.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:
>> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing
>> > braces around initialization of a subobject, which is problematic when
>> > building with `DEVELOPER=YesPlease` which enables `-Werror`:
>> >
>> >     builtin/unpack-objects.c:388:26: error: suggest braces around
>> >         initialization of subobject [-Werror,-Wmissing-braces]
>> >             git_zstream zstream = { 0 };
>> >
>> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)"
>> > -       git_zstream zstream = { 0 };
>> > +       git_zstream zstream = {{ 0 }};
>>
>> Not a comment, just wondering, when should I use "{ { 0 } }" and when
>> should I use "{ 0 }"?
>>
>> I didn't get the error with "Apple clang version 13.0.0
>> (clang-1300.0.29.30)",  because it's
>> a higher version ?
>
> I don't have a good answer. More modern `clang` versions don't seem to
> complain about plain old `{0}` here, but the older `clang` with which
> I'm stuck does complain.

I think, from the language-lawyer perspective, "{ 0 }" is how we
should spell these initialization when we are not using designated
initializers, even when the first member of the struct happens to be
a struct.

The older clang that complains at you is simply buggy, and I think
we had the same issue with older sparse.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Eric Sunshine]

On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:
> >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)"
> >> > -       git_zstream zstream = { 0 };
> >> > +       git_zstream zstream = {{ 0 }};
> >>
> >> Not a comment, just wondering, when should I use "{ { 0 } }" and when
> >> should I use "{ 0 }"?
> >
> > I don't have a good answer. More modern `clang` versions don't seem to
> > complain about plain old `{0}` here, but the older `clang` with which
> > I'm stuck does complain.
>
> I think, from the language-lawyer perspective, "{ 0 }" is how we
> should spell these initialization when we are not using designated
> initializers, even when the first member of the struct happens to be
> a struct.
>
> The older clang that complains at you is simply buggy, and I think
> we had the same issue with older sparse.

I can't tell from your response whether or not you intend to pick up
this patch. I don't disagree that older clang may be considered buggy
in this regard, but older clang versions still exist in the wild, and
we already support them by applying `{{0}}` when appropriate:

    % git grep -n '{ *{ *0 *} *}'
    builtin/merge-file.c:31: xmparam_t xmp = {{0}};
    builtin/worktree.c:262: struct config_set cs = { { 0 } };
    oidset.h:25:#define OIDSET_INIT { { 0 } }
    worktree.c:840: struct config_set cs = { { 0 } };

so the change made by this patch is in line with existing practice on
this project.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 12 2022, Eric Sunshine wrote:

> On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:
>> >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> >> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)"
>> >> > -       git_zstream zstream = { 0 };
>> >> > +       git_zstream zstream = {{ 0 }};
>> >>
>> >> Not a comment, just wondering, when should I use "{ { 0 } }" and when
>> >> should I use "{ 0 }"?
>> >
>> > I don't have a good answer. More modern `clang` versions don't seem to
>> > complain about plain old `{0}` here, but the older `clang` with which
>> > I'm stuck does complain.
>>
>> I think, from the language-lawyer perspective, "{ 0 }" is how we
>> should spell these initialization when we are not using designated
>> initializers, even when the first member of the struct happens to be
>> a struct.
>>
>> The older clang that complains at you is simply buggy, and I think
>> we had the same issue with older sparse.
>
> I can't tell from your response whether or not you intend to pick up
> this patch. I don't disagree that older clang may be considered buggy
> in this regard, but older clang versions still exist in the wild, and
> we already support them by applying `{{0}}` when appropriate:
>
>     % git grep -n '{ *{ *0 *} *}'
>     builtin/merge-file.c:31: xmparam_t xmp = {{0}};

Not so fast :) If you check out "next", does compiling
builtin/merge-file.o there complain on that clang version now? I changed
this to the "{ 0 }" form.

If not I wonder if this has to do with one of git_zstream being
typedef'd, or with the first member being an embedded struct (I couldn't
find another example of that). For the former does the patch at the end
& "make builtin/unpack-objects.o" make it go away?


>     builtin/worktree.c:262: struct config_set cs = { { 0 } };
>     oidset.h:25:#define OIDSET_INIT { { 0 } }
>     worktree.c:840: struct config_set cs = { { 0 } };

Uh, and here are some other examples, so those also warn if you make
them just a "{ 0 }"?

> so the change made by this patch is in line with existing practice on
> this project.

It is nice though to be able to use standard C99 consistently, where a
"{ 0 }" recursively initializes the members, I think that's what your
clang version is doing, it's just complaining about it.

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 ad-hoc test patch referred to above:

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 43789b8ef29..f08092cb26d 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -110,7 +110,7 @@ static void use(int bytes)
  */
 static void *get_data(unsigned long size)
 {
-	git_zstream stream;
+	struct git_zstream stream;
 	unsigned long bufsize = dry_run && size > 8192 ? 8192 : size;
 	void *buf = xmallocz(bufsize);
 
@@ -352,7 +352,7 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,
 }
 
 struct input_zstream_data {
-	git_zstream *zstream;
+	struct git_zstream *zstream;
 	unsigned char buf[8192];
 	int status;
 };
@@ -361,7 +361,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,
 				      unsigned long *readlen)
 {
 	struct input_zstream_data *data = in_stream->data;
-	git_zstream *zstream = data->zstream;
+	struct git_zstream *zstream = data->zstream;
 	void *in = fill(1);
 
 	if (in_stream->is_finished) {
@@ -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 };
+	struct git_zstream zstream = { 0 };
 	struct input_zstream_data data = { 0 };
 	struct input_stream in_stream = {
 		.read = feed_input_zstream,
diff --git a/cache.h b/cache.h
index ac5ab4ef9d3..797f8e4edae 100644
--- a/cache.h
+++ b/cache.h
@@ -18,7 +18,7 @@
 #include "repository.h"
 #include "mem-pool.h"
 
-typedef struct git_zstream {
+struct git_zstream {
 	z_stream z;
 	unsigned long avail_in;
 	unsigned long avail_out;
@@ -26,21 +26,21 @@ typedef struct git_zstream {
 	unsigned long total_out;
 	unsigned char *next_in;
 	unsigned char *next_out;
-} git_zstream;
-
-void git_inflate_init(git_zstream *);
-void git_inflate_init_gzip_only(git_zstream *);
-void git_inflate_end(git_zstream *);
-int git_inflate(git_zstream *, int flush);
-
-void git_deflate_init(git_zstream *, int level);
-void git_deflate_init_gzip(git_zstream *, int level);
-void git_deflate_init_raw(git_zstream *, int level);
-void git_deflate_end(git_zstream *);
-int git_deflate_abort(git_zstream *);
-int git_deflate_end_gently(git_zstream *);
-int git_deflate(git_zstream *, int flush);
-unsigned long git_deflate_bound(git_zstream *, unsigned long);
+};
+
+void git_inflate_init(struct git_zstream *);
+void git_inflate_init_gzip_only(struct git_zstream *);
+void git_inflate_end(struct git_zstream *);
+int git_inflate(struct git_zstream *, int flush);
+
+void git_deflate_init(struct git_zstream *, int level);
+void git_deflate_init_gzip(struct git_zstream *, int level);
+void git_deflate_init_raw(struct git_zstream *, int level);
+void git_deflate_end(struct git_zstream *);
+int git_deflate_abort(struct git_zstream *);
+int git_deflate_end_gently(struct git_zstream *);
+int git_deflate(struct git_zstream *, int flush);
+unsigned long git_deflate_bound(struct git_zstream *, unsigned long);
 
 #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
 #define DTYPE(de)	((de)->d_type)
@@ -1372,7 +1372,7 @@ enum unpack_loose_header_result {
 	ULHR_BAD,
 	ULHR_TOO_LONG,
 };
-enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
+enum unpack_loose_header_result unpack_loose_header(struct git_zstream *stream,
 						    unsigned char *map,
 						    unsigned long mapsize,
 						    void *buffer,

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Eric Sunshine]

On Tue, Jul 12, 2022 at 2:55 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Tue, Jul 12 2022, Eric Sunshine wrote:
> > I can't tell from your response whether or not you intend to pick up
> > this patch. I don't disagree that older clang may be considered buggy
> > in this regard, but older clang versions still exist in the wild, and
> > we already support them by applying `{{0}}` when appropriate:
> >
> >     % git grep -n '{ *{ *0 *} *}'
> >     builtin/merge-file.c:31: xmparam_t xmp = {{0}};
>
> Not so fast :) If you check out "next", does compiling
> builtin/merge-file.o there complain on that clang version now? I changed
> this to the "{ 0 }" form.

No, builtin/merge-file.c doesn't compile, and I discovered that just
after sending the email to which you responded. I haven't yet prepared
a patch for that new instance since I don't know if Junio feels
inclined to pick up such a change.

> If not I wonder if this has to do with one of git_zstream being
> typedef'd, or with the first member being an embedded struct (I couldn't
> find another example of that). For the former does the patch at the end
> & "make builtin/unpack-objects.o" make it go away?

No, the patch you included doesn't make the problem go away (even
after I fixed all the "error: must use 'struct' tag to refer to type
'git_zstream'" errors which showed up throughout the project after
applying your patch).

> >     builtin/worktree.c:262: struct config_set cs = { { 0 } };
> >     oidset.h:25:#define OIDSET_INIT { { 0 } }
> >     worktree.c:840: struct config_set cs = { { 0 } };
>
> Uh, and here are some other examples, so those also warn if you make
> them just a "{ 0 }"?

Yes. (Full disclosure: Even though the two worktree-related instances
come from commits by Stolee, I'm pretty sure I'm the one who asked him
to change them from `{0}` to `{{0}}` during review for this very
reason.)

> > so the change made by this patch is in line with existing practice on
> > this project.
>
> It is nice though to be able to use standard C99 consistently, where a
> "{ 0 }" recursively initializes the members, I think that's what your
> clang version is doing, it's just complaining about it.

Agreed, it would be nice to use plain `{0}`.

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

I'm in favor of this. It would, of course, require extra
special-casing for Apple's clang for which the version number bears no
resemblance to reality since Apple invents their own version numbers.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Jeff King]

On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote:

> > 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.
> 
> I'm in favor of this. It would, of course, require extra
> special-casing for Apple's clang for which the version number bears no
> resemblance to reality since Apple invents their own version numbers.

I got PTSD reading that thread again, but in case anybody wants to dig
into this, I think there are some hints from the last time we discussed
this (starting at the end of this message and the subthread):

  https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/

-Peff

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Eric Sunshine]

On Tue, Jul 12, 2022 at 3:23 AM Jeff King <peff@peff.net> wrote:
> On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote:
> > > 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.
> >
> > I'm in favor of this. It would, of course, require extra
> > special-casing for Apple's clang for which the version number bears no
> > resemblance to reality since Apple invents their own version numbers.
>
> I got PTSD reading that thread again, but in case anybody wants to dig
> into this, I think there are some hints from the last time we discussed
> this (starting at the end of this message and the subthread):
>
>   https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/

Thanks for digging up that link, and sorry for triggering PTSD.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 12 2022, Jeff King wrote:

> On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote:
>
>> > 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.
>> 
>> I'm in favor of this. It would, of course, require extra
>> special-casing for Apple's clang for which the version number bears no
>> resemblance to reality since Apple invents their own version numbers.

FWIW I was imagining just providing that -Wno-* on clang versions <= 11,
not special-casing Apple's in particular.

If you want to make it more strict you can always compare against the
uname, at this point in config.mak.dev we've already sourced
config.mak.uname, so you can guard this with "ifeq ($(uname_S),Darwin)".

Of course that doesn't tell you if it's Apple's clang, just "a clang on
Apple", but it should be close enough not to matter...

> I got PTSD reading that thread again, but in case anybody wants to dig
> into this, I think there are some hints from the last time we discussed
> this (starting at the end of this message and the subthread):
>
>   https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/

Oh yes, the config.mak.dev horror show :)

I have a local patches that carry forward the idea I had in that thread,
i.e. to drop all this version detection insanity and just compile a C
program to detect the compiler.

It takes a bit of doing in the Makefile, but I think the end result is
lovely compared to the status quo. We just do:

	$ head -n 2 config.mak.dev
	include .build/probe/compiler.mak
	include .build/probe/config-mak-dev.mak
	[The rest is all using existing defined variables, no shell magic]

Which is just made with a Makefile by piping this sort of thing to those
.build files:
	
	$ ./.build/probe/config-mak-dev 
	PROBE_COMPILER_NEEDS_std-eq-gnu99 = 1
	PROBE_COMPILER_HAS_Wtautological-constant-out-of-range-compare = 1
	PROBE_COMPILER_HAS_Wextra = 1
	PROBE_COMPILER_HAS_Wpedantic = 1

Which in turn is generated with stand-alone C programs in probe/, which
don't need any of the rest of git:
	
	$ cat probe/config-mak-dev.c
	
	#ifdef PROBE_STANDALONE
	#include <stdlib.h>
	#else
	#include "git-compat-util.h"
	#endif
	
	#include "probe/compiler.h"
	#ifdef __GLIBC__
	#include <gnu/libc-version.h>
	#endif
	
	int probe_config_mak_dev(probe_info_fn_t fn, void *util)
	{
	#ifdef __clang__
	#if __clang_major__ >= 7
		fn(util, "NEEDS_std-eq-gnu99", "1");
	#endif
	#ifndef __has_warning
	#error "Clang version too old to support __has_warning!"
	#endif
	#if __has_warning("-Wtautological-constant-out-of-range-compare")
		fn(util, "HAS_Wtautological-constant-out-of-range-compare", "1");
	#endif
	#if __has_warning("-Wextra")
		fn(util, "HAS_Wextra", "1");
	#endif
	#if __has_warning("-Wpedantic")
		fn(util, "HAS_Wpedantic", "1");
	#endif /* __clang__ */
	
	#elif defined(__GNUC__)
	#if __GNUC__ == 4
		fn(util, "NEEDS_Wno-uninitialized", "1");
	#endif
	#if __GNUC__ >= 5
		fn(util, "HAS_Wpedantic", "1");
	#if __GNUC__ >= 6
		fn(util, "NEEDS_std-eq-gnu99", "1");
		fn(util, "HAS_Wextra", "1");
	#if __GNUC__ >= 10
		fn(util, "HAS_Wno-pedantic-ms-format", "1");
	#endif /* >= 10 */
	#endif /* >= 6 */
	#endif /* >= 5 */
	
	#elif defined(__IBMC__)
	
	#else
		return -1;
	#endif
		return 0;
	}
	
	#ifdef PROBE_STANDALONE
	#include <stdio.h>
	#include "probe/print.h"
	
	int main(void)
	{
		struct probe_print_data data = {
			.prefix = "PROBE_COMPILER_",
		};
	
		if (probe_config_mak_dev(probe_print, &data) < 0)
			fprintf(stderr, "warning: unable to detect compiler type and version\n");
		return 0;
	}
	#endif

The compilation is then triggered by the include in config.mak.dev,
which has a corresponding rule that creates the C program, then the
generated *.mak, so once we do it once we're only ever including an
already generated text file.

It takes a bit of doing in the Makfile, since we need to e.g. declare
that "artifacts-tar", "check-docs" etc. don't want to build this C
program to "configure bootstrap" even if under DEVELOPER=1, i.e. we need
to know which target(s) we'll run to compile C code.

But that has the bonus benefit of making those faster, as now we'll
e.g. $(shell detect-compiler), generate the version info etc., only to
run "$(MAKE) -C Documentation/ ..." or whatever.

I can clean it up for submission if there's interest.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> >     % git grep -n '{ *{ *0 *} *}'
>> >     builtin/merge-file.c:31: xmparam_t xmp = {{0}};
>>
>> Not so fast :) If you check out "next", does compiling
>> builtin/merge-file.o there complain on that clang version now? I changed
>> this to the "{ 0 }" form.
>
> No, builtin/merge-file.c doesn't compile, and I discovered that just
> after sending the email to which you responded. I haven't yet prepared
> a patch for that new instance since I don't know if Junio feels
> inclined to pick up such a change.

Wait, what do you mean by "doesn't compile"?  The compiler totally
chokes on "{ 0 } recursively zero initializes" idiom and does not
know what binary to produce, or it merely warns even though it knows
what to do with the code, but because we choose to give -Werror, it
is stopped from producing a binary?

>> It is nice though to be able to use standard C99 consistently, where a
>> "{ 0 }" recursively initializes the members, I think that's what your
>> clang version is doing, it's just complaining about it.
>
> Agreed, it would be nice to use plain `{0}`.
>
>> 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.
>
> I'm in favor of this. It would, of course, require extra
> special-casing for Apple's clang for which the version number bears no
> resemblance to reality since Apple invents their own version numbers.

I guess from this that you meant "we get an erroneous warning".  If
so, I am in favor of squelching the warning.

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Jeff King]

On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> I'm in favor of this. It would, of course, require extra
> >> special-casing for Apple's clang for which the version number bears no
> >> resemblance to reality since Apple invents their own version numbers.
> 
> FWIW I was imagining just providing that -Wno-* on clang versions <= 11,
> not special-casing Apple's in particular.

It's not about special-casing Apple in particular. It's that our
detect-compiler script does not understand which version of clang is
used for Apple's compiler. Their version numbers are totally mismatched.

So you either have to say "turn off this warning for clang totally", or
do the wrong thing when Apple's compiler is in use.

> I have a local patches that carry forward the idea I had in that thread,
> i.e. to drop all this version detection insanity and just compile a C
> program to detect the compiler.

Hmm. I prefer your earlier suggestion to use "$(CC) -E". This tool has
to run on every invocation of "make", so the lighter-weight it is, the
better. You say later...

> The compilation is then triggered by the include in config.mak.dev,
> which has a corresponding rule that creates the C program, then the
> generated *.mak, so once we do it once we're only ever including an
> already generated text file.

but that implies we don't have a dependency on the compiler itself.
You'd want to at least depend on the name of the compiler. But that
would also miss if running "clang" changes which version of clang you're
running. I know upgrading your compiler is rare-ish, but this is exactly
the kind of thing I expect to bite at the most annoying time (when
you're switching around versions to try to figure out how they behave).

> 	#ifdef __clang__
> 	#if __clang_major__ >= 7
> 		fn(util, "NEEDS_std-eq-gnu99", "1");
> 	#endif

This is still gross version detection, but I don't think we can avoid
it. However...

> 	#if __has_warning("-Wextra")
> 		fn(util, "HAS_Wextra", "1");
> 	#endif

...this is much nicer. It could still be implemented purely via "-E", as
far as I can see, like:

  #if __has_warning("-Wextra")
  HAS_Wextra = 1
  #endif

But then we end up having to do version comparisons for gcc anyway:

> 	#if __GNUC__ >= 6
> 		fn(util, "NEEDS_std-eq-gnu99", "1");
> 		fn(util, "HAS_Wextra", "1");

so it feels like we're back where we started. You've just encoded the
version checks in a different spot.

I dunno. I don't find this significantly less gross than the status quo.
I don't mind getting the version via "-E" rather than "-v", but whether
the policy logic is in cpp, or in shell, or in the Makefile, it still
needs to exist. Putting it in cpp allows using has_warning(), but since
that isn't available everywhere, I'm not sure it buys us much.

-Peff

Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 14 2022, Jeff King wrote:

> On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> I'm in favor of this. It would, of course, require extra
>> >> special-casing for Apple's clang for which the version number bears no
>> >> resemblance to reality since Apple invents their own version numbers.
>> 
>> FWIW I was imagining just providing that -Wno-* on clang versions <= 11,
>> not special-casing Apple's in particular.
>
> It's not about special-casing Apple in particular. It's that our
> detect-compiler script does not understand which version of clang is
> used for Apple's compiler. Their version numbers are totally mismatched.
>
> So you either have to say "turn off this warning for clang totally", or
> do the wrong thing when Apple's compiler is in use.

I missed the part where we don't even detect the version in that case,
but anyway, just turning it off on clang && OSX seems fine. This is
-Wmissing-braces, we'll catch it elsewhere (CI etc.)

>> I have a local patches that carry forward the idea I had in that thread,
>> i.e. to drop all this version detection insanity and just compile a C
>> program to detect the compiler.
>
> Hmm. I prefer your earlier suggestion to use "$(CC) -E". This tool has
> to run on every invocation of "make", so the lighter-weight it is, the
> better. You say later...

We only compile & run the binary once, then save the result to a *.mak
file, the intention is to get rid of running these binaries or script on
every single invocation.

>> The compilation is then triggered by the include in config.mak.dev,
>> which has a corresponding rule that creates the C program, then the
>> generated *.mak, so once we do it once we're only ever including an
>> already generated text file.
>
> but that implies we don't have a dependency on the compiler itself.
> You'd want to at least depend on the name of the compiler. But that
> would also miss if running "clang" changes which version of clang you're
> running. I know upgrading your compiler is rare-ish, but this is exactly
> the kind of thing I expect to bite at the most annoying time (when
> you're switching around versions to try to figure out how they behave).

That's true, I considered that OK, and still do. I.e. we use this for
config.mak, where we are opting you in to never flags.

If your compiler name changes your GIT-BUILD-OPTIONS will change, so
we'll re-build and re-detect.

But yes, if your CC=gcc and you change gcc from under us we won't
re-detect and re-build.

But isn't that fine? You just won't get newer flags, you might downgrade
your gcc and get an error, but generally speaking make-based systems
really don't try to detect "anything on the OS" changed.

Still, I could easily add something where we one-off run "command -v
$(CC)", save that to a file, and then add that to our "make" dependency
tree.

So then if the compiler binary's mtime changes we'd re-build, and we
still wouldn't run something on every invocation.

It just seems to be quite pedantic about correctness, is all :)

>> 	#ifdef __clang__
>> 	#if __clang_major__ >= 7
>> 		fn(util, "NEEDS_std-eq-gnu99", "1");
>> 	#endif
>
> This is still gross version detection, but I don't think we can avoid
> it. However...

Yes, this part isn't really nicer, although with this method we could
also avoid this sort of thing by just trying the flag out & caching
that, but this seemed OK.

>> 	#if __has_warning("-Wextra")
>> 		fn(util, "HAS_Wextra", "1");
>> 	#endif
>
> ...this is much nicer. It could still be implemented purely via "-E", as
> far as I can see, like:
>
>   #if __has_warning("-Wextra")
>   HAS_Wextra = 1
>   #endif

*nod*, I wish GCC had that. 

> But then we end up having to do version comparisons for gcc anyway:
>
>> 	#if __GNUC__ >= 6
>> 		fn(util, "NEEDS_std-eq-gnu99", "1");
>> 		fn(util, "HAS_Wextra", "1");
>
> so it feels like we're back where we started. You've just encoded the
> version checks in a different spot.

We're really not, the reason I did this was because i tried to add
support for xlc and suncc to this script, we don't even parse
detect-compiler on Solaris now, as its /bin/sh isn't compatible with
it. So to begin with we'd need to hoist the "shell detection and script
generation" out ...

> I dunno. I don't find this significantly less gross than the status quo.
> I don't mind getting the version via "-E" rather than "-v", but whether
> the policy logic is in cpp, or in shell, or in the Makefile, it still
> needs to exist. Putting it in cpp allows using has_warning(), but since
> that isn't available everywhere, I'm not sure it buys us much.

Compilers universally support "who and what am I?" via their native
macros, but we're trying to do it the really hard way via parsing
version output.