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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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,