On Tue, Jul 12 2022, Eric Sunshine wrote:
> On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>> > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@xxxxxxxxx> wrote:
>> >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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,
