{"thread":{"id":"58127","subject":"[PATCH] unpack-objects: fix compilation warning/error due to missing braces","startedAt":"2022-07-10T08:14:06Z","lastAt":"2022-07-15T08:29:00Z","messageCount":13,"participants":["Eric Sunshine","Han Xin","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458705","messageId":"20220710081135.74964-1-sunshine@sunshineco.com","threadId":"58127","inReplyTo":null,"subject":"[PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-10T08:11:35Z","receivedAt":"2022-07-10T08:14:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing\nbraces around initialization of a subobject, which is problematic when\nbuilding with `DEVELOPER=YesPlease` which enables `-Werror`:\n\n    builtin/unpack-objects.c:388:26: error: suggest braces around\n        initialization of subobject [-Werror,-Wmissing-braces]\n            git_zstream zstream = { 0 };\n\n[1]: `cc --version` => \"Apple LLVM version 10.0.0 (clang-1000.10.44.4)\"\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nNotes:\n    This is atop 'hx/unpack-streaming' which is already in 'next'.\n    All the CI builds are fine with this change.\n\n    As I understand it, this should be a safe change; the fields which\n    follow `z_stream z` in `git_zstream` will be initialized to zero\n    since the first field has an explicit initializer.\n\n builtin/unpack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 43789b8ef2..c606c92e37 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,\n \n static void stream_blob(unsigned long size, unsigned nr)\n {\n-\tgit_zstream zstream = { 0 };\n+\tgit_zstream zstream = {{ 0 }};\n \tstruct input_zstream_data data = { 0 };\n \tstruct input_stream in_stream = {\n \t\t.read = feed_input_zstream,\n-- \n2.37.0.236.gcef32db0b6.dirty\n\n"},{"id":"458743","messageId":"CAO0brD0PBXDqe2HDdjg1ZhXWoYZihQ0=SY80UR+Cy3xRqqH8Sg@mail.gmail.com","threadId":"58127","inReplyTo":"20220710081135.74964-1-sunshine@sunshineco.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Han Xin","fromEmail":"chiyutianyi@gmail.com","sentAt":"2022-07-11T02:00:35Z","receivedAt":"2022-07-11T02:00:50Z","isPatch":true,"sender":{"key":"chiyutianyi@gmail.com","avatar":null},"body":"On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing\n> braces around initialization of a subobject, which is problematic when\n> building with `DEVELOPER=YesPlease` which enables `-Werror`:\n>\n>     builtin/unpack-objects.c:388:26: error: suggest braces around\n>         initialization of subobject [-Werror,-Wmissing-braces]\n>             git_zstream zstream = { 0 };\n>\n> [1]: `cc --version` => \"Apple LLVM version 10.0.0 (clang-1000.10.44.4)\"\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>\n> Notes:\n>     This is atop 'hx/unpack-streaming' which is already in 'next'.\n>     All the CI builds are fine with this change.\n>\n>     As I understand it, this should be a safe change; the fields which\n>     follow `z_stream z` in `git_zstream` will be initialized to zero\n>     since the first field has an explicit initializer.\n>\n>  builtin/unpack-objects.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index 43789b8ef2..c606c92e37 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,\n>\n>  static void stream_blob(unsigned long size, unsigned nr)\n>  {\n> -       git_zstream zstream = { 0 };\n> +       git_zstream zstream = {{ 0 }};\n>         struct input_zstream_data data = { 0 };\n>         struct input_stream in_stream = {\n>                 .read = feed_input_zstream,\n> --\n> 2.37.0.236.gcef32db0b6.dirty\n>\n\nNot a comment, just wondering, when should I use \"{ { 0 } }\" and when\nshould I use \"{ 0 }\"?\n\nI didn't get the error with \"Apple clang version 13.0.0\n(clang-1300.0.29.30)\",  because it's\na higher version ?\n\nThanks.\n-Han Xin\n"},{"id":"458744","messageId":"CAPig+cQJWgerk08j=1b=aWRZsKBu3BnEACQuiqktU4BwzM-xaA@mail.gmail.com","threadId":"58127","inReplyTo":"CAO0brD0PBXDqe2HDdjg1ZhXWoYZihQ0=SY80UR+Cy3xRqqH8Sg@mail.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-11T02:41:12Z","receivedAt":"2022-07-11T02:41:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:\n> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing\n> > braces around initialization of a subobject, which is problematic when\n> > building with `DEVELOPER=YesPlease` which enables `-Werror`:\n> >\n> >     builtin/unpack-objects.c:388:26: error: suggest braces around\n> >         initialization of subobject [-Werror,-Wmissing-braces]\n> >             git_zstream zstream = { 0 };\n> >\n> > [1]: `cc --version` => \"Apple LLVM version 10.0.0 (clang-1000.10.44.4)\"\n> > -       git_zstream zstream = { 0 };\n> > +       git_zstream zstream = {{ 0 }};\n>\n> Not a comment, just wondering, when should I use \"{ { 0 } }\" and when\n> should I use \"{ 0 }\"?\n>\n> I didn't get the error with \"Apple clang version 13.0.0\n> (clang-1300.0.29.30)\",  because it's\n> a higher version ?\n\nI don't have a good answer. More modern `clang` versions don't seem to\ncomplain about plain old `{0}` here, but the older `clang` with which\nI'm stuck does complain. Aside from actually building the project with\nan older `clang` (or older Apple-specific `clang`), it may be\nsufficient to inspect the structure that's being initialized to see if\nthe first element is itself a subobject. However, I'm not sure it's\nworth the effort to do so considering how rare this problem seems to\nbe.\n"},{"id":"458749","messageId":"xmqq7d4kp8l6.fsf@gitster.g","threadId":"58127","inReplyTo":"CAPig+cQJWgerk08j=1b=aWRZsKBu3BnEACQuiqktU4BwzM-xaA@mail.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-11T04:38:29Z","receivedAt":"2022-07-11T04:38:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:\n>> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing\n>> > braces around initialization of a subobject, which is problematic when\n>> > building with `DEVELOPER=YesPlease` which enables `-Werror`:\n>> >\n>> >     builtin/unpack-objects.c:388:26: error: suggest braces around\n>> >         initialization of subobject [-Werror,-Wmissing-braces]\n>> >             git_zstream zstream = { 0 };\n>> >\n>> > [1]: `cc --version` => \"Apple LLVM version 10.0.0 (clang-1000.10.44.4)\"\n>> > -       git_zstream zstream = { 0 };\n>> > +       git_zstream zstream = {{ 0 }};\n>>\n>> Not a comment, just wondering, when should I use \"{ { 0 } }\" and when\n>> should I use \"{ 0 }\"?\n>>\n>> I didn't get the error with \"Apple clang version 13.0.0\n>> (clang-1300.0.29.30)\",  because it's\n>> a higher version ?\n>\n> I don't have a good answer. More modern `clang` versions don't seem to\n> complain about plain old `{0}` here, but the older `clang` with which\n> I'm stuck does complain.\n\nI think, from the language-lawyer perspective, \"{ 0 }\" is how we\nshould spell these initialization when we are not using designated\ninitializers, even when the first member of the struct happens to be\na struct.\n\nThe older clang that complains at you is simply buggy, and I think\nwe had the same issue with older sparse.\n\n"},{"id":"458847","messageId":"CAPig+cQMJcUc4gpRDpR=Q8M44rTjUA7SWgXNmzrnDH7V12z0dQ@mail.gmail.com","threadId":"58127","inReplyTo":"xmqq7d4kp8l6.fsf@gitster.g","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-12T06:28:48Z","receivedAt":"2022-07-12T06:29:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:\n> >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> >> > [1]: `cc --version` => \"Apple LLVM version 10.0.0 (clang-1000.10.44.4)\"\n> >> > -       git_zstream zstream = { 0 };\n> >> > +       git_zstream zstream = {{ 0 }};\n> >>\n> >> Not a comment, just wondering, when should I use \"{ { 0 } }\" and when\n> >> should I use \"{ 0 }\"?\n> >\n> > I don't have a good answer. More modern `clang` versions don't seem to\n> > complain about plain old `{0}` here, but the older `clang` with which\n> > I'm stuck does complain.\n>\n> I think, from the language-lawyer perspective, \"{ 0 }\" is how we\n> should spell these initialization when we are not using designated\n> initializers, even when the first member of the struct happens to be\n> a struct.\n>\n> The older clang that complains at you is simply buggy, and I think\n> we had the same issue with older sparse.\n\nI can't tell from your response whether or not you intend to pick up\nthis patch. I don't disagree that older clang may be considered buggy\nin this regard, but older clang versions still exist in the wild, and\nwe already support them by applying `{{0}}` when appropriate:\n\n    % git grep -n '{ *{ *0 *} *}'\n    builtin/merge-file.c:31: xmparam_t xmp = {{0}};\n    builtin/worktree.c:262: struct config_set cs = { { 0 } };\n    oidset.h:25:#define OIDSET_INIT { { 0 } }\n    worktree.c:840: struct config_set cs = { { 0 } };\n\nso the change made by this patch is in line with existing practice on\nthis project.\n"},{"id":"458852","messageId":"220712.86lesy6cri.gmgdl@evledraar.gmail.com","threadId":"58127","inReplyTo":"CAPig+cQMJcUc4gpRDpR=Q8M44rTjUA7SWgXNmzrnDH7V12z0dQ@mail.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-12T06:41:49Z","receivedAt":"2022-07-12T06:55:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 12 2022, Eric Sunshine wrote:\n\n> On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote:\n>> >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> >> > [1]: `cc --version` => \"Apple LLVM version 10.0.0 (clang-1000.10.44.4)\"\n>> >> > -       git_zstream zstream = { 0 };\n>> >> > +       git_zstream zstream = {{ 0 }};\n>> >>\n>> >> Not a comment, just wondering, when should I use \"{ { 0 } }\" and when\n>> >> should I use \"{ 0 }\"?\n>> >\n>> > I don't have a good answer. More modern `clang` versions don't seem to\n>> > complain about plain old `{0}` here, but the older `clang` with which\n>> > I'm stuck does complain.\n>>\n>> I think, from the language-lawyer perspective, \"{ 0 }\" is how we\n>> should spell these initialization when we are not using designated\n>> initializers, even when the first member of the struct happens to be\n>> a struct.\n>>\n>> The older clang that complains at you is simply buggy, and I think\n>> we had the same issue with older sparse.\n>\n> I can't tell from your response whether or not you intend to pick up\n> this patch. I don't disagree that older clang may be considered buggy\n> in this regard, but older clang versions still exist in the wild, and\n> we already support them by applying `{{0}}` when appropriate:\n>\n>     % git grep -n '{ *{ *0 *} *}'\n>     builtin/merge-file.c:31: xmparam_t xmp = {{0}};\n\nNot so fast :) If you check out \"next\", does compiling\nbuiltin/merge-file.o there complain on that clang version now? I changed\nthis to the \"{ 0 }\" form.\n\nIf not I wonder if this has to do with one of git_zstream being\ntypedef'd, or with the first member being an embedded struct (I couldn't\nfind another example of that). For the former does the patch at the end\n& \"make builtin/unpack-objects.o\" make it go away?\n\n\n>     builtin/worktree.c:262: struct config_set cs = { { 0 } };\n>     oidset.h:25:#define OIDSET_INIT { { 0 } }\n>     worktree.c:840: struct config_set cs = { { 0 } };\n\nUh, and here are some other examples, so those also warn if you make\nthem just a \"{ 0 }\"?\n\n> so the change made by this patch is in line with existing practice on\n> this project.\n\nIt is nice though to be able to use standard C99 consistently, where a\n\"{ 0 }\" recursively initializes the members, I think that's what your\nclang version is doing, it's just complaining about it.\n\nSince this is only a warning, and only a practical issue with -Werror I\nwonder if a config.mak.dev change wouldn't be better, i.e. to provide a\n-Wno-missing-braces for this older clang version.\n\nThe ad-hoc test patch referred to above:\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 43789b8ef29..f08092cb26d 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -110,7 +110,7 @@ static void use(int bytes)\n  */\n static void *get_data(unsigned long size)\n {\n-\tgit_zstream stream;\n+\tstruct git_zstream stream;\n \tunsigned long bufsize = dry_run && size > 8192 ? 8192 : size;\n \tvoid *buf = xmallocz(bufsize);\n \n@@ -352,7 +352,7 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,\n }\n \n struct input_zstream_data {\n-\tgit_zstream *zstream;\n+\tstruct git_zstream *zstream;\n \tunsigned char buf[8192];\n \tint status;\n };\n@@ -361,7 +361,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,\n \t\t\t\t      unsigned long *readlen)\n {\n \tstruct input_zstream_data *data = in_stream->data;\n-\tgit_zstream *zstream = data->zstream;\n+\tstruct git_zstream *zstream = data->zstream;\n \tvoid *in = fill(1);\n \n \tif (in_stream->is_finished) {\n@@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,\n \n static void stream_blob(unsigned long size, unsigned nr)\n {\n-\tgit_zstream zstream = { 0 };\n+\tstruct git_zstream zstream = { 0 };\n \tstruct input_zstream_data data = { 0 };\n \tstruct input_stream in_stream = {\n \t\t.read = feed_input_zstream,\ndiff --git a/cache.h b/cache.h\nindex ac5ab4ef9d3..797f8e4edae 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -18,7 +18,7 @@\n #include \"repository.h\"\n #include \"mem-pool.h\"\n \n-typedef struct git_zstream {\n+struct git_zstream {\n \tz_stream z;\n \tunsigned long avail_in;\n \tunsigned long avail_out;\n@@ -26,21 +26,21 @@ typedef struct git_zstream {\n \tunsigned long total_out;\n \tunsigned char *next_in;\n \tunsigned char *next_out;\n-} git_zstream;\n-\n-void git_inflate_init(git_zstream *);\n-void git_inflate_init_gzip_only(git_zstream *);\n-void git_inflate_end(git_zstream *);\n-int git_inflate(git_zstream *, int flush);\n-\n-void git_deflate_init(git_zstream *, int level);\n-void git_deflate_init_gzip(git_zstream *, int level);\n-void git_deflate_init_raw(git_zstream *, int level);\n-void git_deflate_end(git_zstream *);\n-int git_deflate_abort(git_zstream *);\n-int git_deflate_end_gently(git_zstream *);\n-int git_deflate(git_zstream *, int flush);\n-unsigned long git_deflate_bound(git_zstream *, unsigned long);\n+};\n+\n+void git_inflate_init(struct git_zstream *);\n+void git_inflate_init_gzip_only(struct git_zstream *);\n+void git_inflate_end(struct git_zstream *);\n+int git_inflate(struct git_zstream *, int flush);\n+\n+void git_deflate_init(struct git_zstream *, int level);\n+void git_deflate_init_gzip(struct git_zstream *, int level);\n+void git_deflate_init_raw(struct git_zstream *, int level);\n+void git_deflate_end(struct git_zstream *);\n+int git_deflate_abort(struct git_zstream *);\n+int git_deflate_end_gently(struct git_zstream *);\n+int git_deflate(struct git_zstream *, int flush);\n+unsigned long git_deflate_bound(struct git_zstream *, unsigned long);\n \n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\n@@ -1372,7 +1372,7 @@ enum unpack_loose_header_result {\n \tULHR_BAD,\n \tULHR_TOO_LONG,\n };\n-enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n+enum unpack_loose_header_result unpack_loose_header(struct git_zstream *stream,\n \t\t\t\t\t\t    unsigned char *map,\n \t\t\t\t\t\t    unsigned long mapsize,\n \t\t\t\t\t\t    void *buffer,\n"},{"id":"458859","messageId":"CAPig+cSgNB=SzAZLhXvteSYmy0HvJh+qWHMYyBxcX_EA9__u4A@mail.gmail.com","threadId":"58127","inReplyTo":"220712.86lesy6cri.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-12T07:13:50Z","receivedAt":"2022-07-12T07:14:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 12, 2022 at 2:55 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Tue, Jul 12 2022, Eric Sunshine wrote:\n> > I can't tell from your response whether or not you intend to pick up\n> > this patch. I don't disagree that older clang may be considered buggy\n> > in this regard, but older clang versions still exist in the wild, and\n> > we already support them by applying `{{0}}` when appropriate:\n> >\n> >     % git grep -n '{ *{ *0 *} *}'\n> >     builtin/merge-file.c:31: xmparam_t xmp = {{0}};\n>\n> Not so fast :) If you check out \"next\", does compiling\n> builtin/merge-file.o there complain on that clang version now? I changed\n> this to the \"{ 0 }\" form.\n\nNo, builtin/merge-file.c doesn't compile, and I discovered that just\nafter sending the email to which you responded. I haven't yet prepared\na patch for that new instance since I don't know if Junio feels\ninclined to pick up such a change.\n\n> If not I wonder if this has to do with one of git_zstream being\n> typedef'd, or with the first member being an embedded struct (I couldn't\n> find another example of that). For the former does the patch at the end\n> & \"make builtin/unpack-objects.o\" make it go away?\n\nNo, the patch you included doesn't make the problem go away (even\nafter I fixed all the \"error: must use 'struct' tag to refer to type\n'git_zstream'\" errors which showed up throughout the project after\napplying your patch).\n\n> >     builtin/worktree.c:262: struct config_set cs = { { 0 } };\n> >     oidset.h:25:#define OIDSET_INIT { { 0 } }\n> >     worktree.c:840: struct config_set cs = { { 0 } };\n>\n> Uh, and here are some other examples, so those also warn if you make\n> them just a \"{ 0 }\"?\n\nYes. (Full disclosure: Even though the two worktree-related instances\ncome from commits by Stolee, I'm pretty sure I'm the one who asked him\nto change them from `{0}` to `{{0}}` during review for this very\nreason.)\n\n> > so the change made by this patch is in line with existing practice on\n> > this project.\n>\n> It is nice though to be able to use standard C99 consistently, where a\n> \"{ 0 }\" recursively initializes the members, I think that's what your\n> clang version is doing, it's just complaining about it.\n\nAgreed, it would be nice to use plain `{0}`.\n\n> Since this is only a warning, and only a practical issue with -Werror I\n> wonder if a config.mak.dev change wouldn't be better, i.e. to provide a\n> -Wno-missing-braces for this older clang version.\n\nI'm in favor of this. It would, of course, require extra\nspecial-casing for Apple's clang for which the version number bears no\nresemblance to reality since Apple invents their own version numbers.\n"},{"id":"458861","messageId":"Ys0hhYjPExuNWynE@coredump.intra.peff.net","threadId":"58127","inReplyTo":"CAPig+cSgNB=SzAZLhXvteSYmy0HvJh+qWHMYyBxcX_EA9__u4A@mail.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-12T07:23:49Z","receivedAt":"2022-07-12T07:23:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote:\n\n> > Since this is only a warning, and only a practical issue with -Werror I\n> > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a\n> > -Wno-missing-braces for this older clang version.\n> \n> I'm in favor of this. It would, of course, require extra\n> special-casing for Apple's clang for which the version number bears no\n> resemblance to reality since Apple invents their own version numbers.\n\nI got PTSD reading that thread again, but in case anybody wants to dig\ninto this, I think there are some hints from the last time we discussed\nthis (starting at the end of this message and the subthread):\n\n  https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"458862","messageId":"CAPig+cQeC4_+22rzHFtdvNwL2PqpTLtS-32t0hwcemNKOA36bQ@mail.gmail.com","threadId":"58127","inReplyTo":"Ys0hhYjPExuNWynE@coredump.intra.peff.net","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-12T07:33:58Z","receivedAt":"2022-07-12T07:34:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 12, 2022 at 3:23 AM Jeff King <peff@peff.net> wrote:\n> On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote:\n> > > Since this is only a warning, and only a practical issue with -Werror I\n> > > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a\n> > > -Wno-missing-braces for this older clang version.\n> >\n> > I'm in favor of this. It would, of course, require extra\n> > special-casing for Apple's clang for which the version number bears no\n> > resemblance to reality since Apple invents their own version numbers.\n>\n> I got PTSD reading that thread again, but in case anybody wants to dig\n> into this, I think there are some hints from the last time we discussed\n> this (starting at the end of this message and the subthread):\n>\n>   https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/\n\nThanks for digging up that link, and sorry for triggering PTSD.\n"},{"id":"458867","messageId":"220712.864jzm65mk.gmgdl@evledraar.gmail.com","threadId":"58127","inReplyTo":"Ys0hhYjPExuNWynE@coredump.intra.peff.net","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-12T09:16:10Z","receivedAt":"2022-07-12T09:29:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 12 2022, Jeff King wrote:\n\n> On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote:\n>\n>> > Since this is only a warning, and only a practical issue with -Werror I\n>> > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a\n>> > -Wno-missing-braces for this older clang version.\n>> \n>> I'm in favor of this. It would, of course, require extra\n>> special-casing for Apple's clang for which the version number bears no\n>> resemblance to reality since Apple invents their own version numbers.\n\nFWIW I was imagining just providing that -Wno-* on clang versions <= 11,\nnot special-casing Apple's in particular.\n\nIf you want to make it more strict you can always compare against the\nuname, at this point in config.mak.dev we've already sourced\nconfig.mak.uname, so you can guard this with \"ifeq ($(uname_S),Darwin)\".\n\nOf course that doesn't tell you if it's Apple's clang, just \"a clang on\nApple\", but it should be close enough not to matter...\n\n> I got PTSD reading that thread again, but in case anybody wants to dig\n> into this, I think there are some hints from the last time we discussed\n> this (starting at the end of this message and the subthread):\n>\n>   https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/\n\nOh yes, the config.mak.dev horror show :)\n\nI have a local patches that carry forward the idea I had in that thread,\ni.e. to drop all this version detection insanity and just compile a C\nprogram to detect the compiler.\n\nIt takes a bit of doing in the Makefile, but I think the end result is\nlovely compared to the status quo. We just do:\n\n\t$ head -n 2 config.mak.dev\n\tinclude .build/probe/compiler.mak\n\tinclude .build/probe/config-mak-dev.mak\n\t[The rest is all using existing defined variables, no shell magic]\n\nWhich is just made with a Makefile by piping this sort of thing to those\n.build files:\n\t\n\t$ ./.build/probe/config-mak-dev \n\tPROBE_COMPILER_NEEDS_std-eq-gnu99 = 1\n\tPROBE_COMPILER_HAS_Wtautological-constant-out-of-range-compare = 1\n\tPROBE_COMPILER_HAS_Wextra = 1\n\tPROBE_COMPILER_HAS_Wpedantic = 1\n\nWhich in turn is generated with stand-alone C programs in probe/, which\ndon't need any of the rest of git:\n\t\n\t$ cat probe/config-mak-dev.c\n\t\n\t#ifdef PROBE_STANDALONE\n\t#include <stdlib.h>\n\t#else\n\t#include \"git-compat-util.h\"\n\t#endif\n\t\n\t#include \"probe/compiler.h\"\n\t#ifdef __GLIBC__\n\t#include <gnu/libc-version.h>\n\t#endif\n\t\n\tint probe_config_mak_dev(probe_info_fn_t fn, void *util)\n\t{\n\t#ifdef __clang__\n\t#if __clang_major__ >= 7\n\t\tfn(util, \"NEEDS_std-eq-gnu99\", \"1\");\n\t#endif\n\t#ifndef __has_warning\n\t#error \"Clang version too old to support __has_warning!\"\n\t#endif\n\t#if __has_warning(\"-Wtautological-constant-out-of-range-compare\")\n\t\tfn(util, \"HAS_Wtautological-constant-out-of-range-compare\", \"1\");\n\t#endif\n\t#if __has_warning(\"-Wextra\")\n\t\tfn(util, \"HAS_Wextra\", \"1\");\n\t#endif\n\t#if __has_warning(\"-Wpedantic\")\n\t\tfn(util, \"HAS_Wpedantic\", \"1\");\n\t#endif /* __clang__ */\n\t\n\t#elif defined(__GNUC__)\n\t#if __GNUC__ == 4\n\t\tfn(util, \"NEEDS_Wno-uninitialized\", \"1\");\n\t#endif\n\t#if __GNUC__ >= 5\n\t\tfn(util, \"HAS_Wpedantic\", \"1\");\n\t#if __GNUC__ >= 6\n\t\tfn(util, \"NEEDS_std-eq-gnu99\", \"1\");\n\t\tfn(util, \"HAS_Wextra\", \"1\");\n\t#if __GNUC__ >= 10\n\t\tfn(util, \"HAS_Wno-pedantic-ms-format\", \"1\");\n\t#endif /* >= 10 */\n\t#endif /* >= 6 */\n\t#endif /* >= 5 */\n\t\n\t#elif defined(__IBMC__)\n\t\n\t#else\n\t\treturn -1;\n\t#endif\n\t\treturn 0;\n\t}\n\t\n\t#ifdef PROBE_STANDALONE\n\t#include <stdio.h>\n\t#include \"probe/print.h\"\n\t\n\tint main(void)\n\t{\n\t\tstruct probe_print_data data = {\n\t\t\t.prefix = \"PROBE_COMPILER_\",\n\t\t};\n\t\n\t\tif (probe_config_mak_dev(probe_print, &data) < 0)\n\t\t\tfprintf(stderr, \"warning: unable to detect compiler type and version\\n\");\n\t\treturn 0;\n\t}\n\t#endif\n\nThe compilation is then triggered by the include in config.mak.dev,\nwhich has a corresponding rule that creates the C program, then the\ngenerated *.mak, so once we do it once we're only ever including an\nalready generated text file.\n\nIt takes a bit of doing in the Makfile, since we need to e.g. declare\nthat \"artifacts-tar\", \"check-docs\" etc. don't want to build this C\nprogram to \"configure bootstrap\" even if under DEVELOPER=1, i.e. we need\nto know which target(s) we'll run to compile C code.\n\nBut that has the bonus benefit of making those faster, as now we'll\ne.g. $(shell detect-compiler), generate the version info etc., only to\nrun \"$(MAKE) -C Documentation/ ...\" or whatever.\n\nI can clean it up for submission if there's interest.\n"},{"id":"458892","messageId":"xmqqczeaie2k.fsf@gitster.g","threadId":"58127","inReplyTo":"CAPig+cSgNB=SzAZLhXvteSYmy0HvJh+qWHMYyBxcX_EA9__u4A@mail.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-12T14:46:27Z","receivedAt":"2022-07-12T14:46:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> >     % git grep -n '{ *{ *0 *} *}'\n>> >     builtin/merge-file.c:31: xmparam_t xmp = {{0}};\n>>\n>> Not so fast :) If you check out \"next\", does compiling\n>> builtin/merge-file.o there complain on that clang version now? I changed\n>> this to the \"{ 0 }\" form.\n>\n> No, builtin/merge-file.c doesn't compile, and I discovered that just\n> after sending the email to which you responded. I haven't yet prepared\n> a patch for that new instance since I don't know if Junio feels\n> inclined to pick up such a change.\n\nWait, what do you mean by \"doesn't compile\"?  The compiler totally\nchokes on \"{ 0 } recursively zero initializes\" idiom and does not\nknow what binary to produce, or it merely warns even though it knows\nwhat to do with the code, but because we choose to give -Werror, it\nis stopped from producing a binary?\n\n>> It is nice though to be able to use standard C99 consistently, where a\n>> \"{ 0 }\" recursively initializes the members, I think that's what your\n>> clang version is doing, it's just complaining about it.\n>\n> Agreed, it would be nice to use plain `{0}`.\n>\n>> Since this is only a warning, and only a practical issue with -Werror I\n>> wonder if a config.mak.dev change wouldn't be better, i.e. to provide a\n>> -Wno-missing-braces for this older clang version.\n>\n> I'm in favor of this. It would, of course, require extra\n> special-casing for Apple's clang for which the version number bears no\n> resemblance to reality since Apple invents their own version numbers.\n\nI guess from this that you meant \"we get an erroneous warning\".  If\nso, I am in favor of squelching the warning.\n"},{"id":"459097","messageId":"YtCQl1oinrVnfa+6@coredump.intra.peff.net","threadId":"58127","inReplyTo":"220712.864jzm65mk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-14T21:54:31Z","receivedAt":"2022-07-14T21:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> I'm in favor of this. It would, of course, require extra\n> >> special-casing for Apple's clang for which the version number bears no\n> >> resemblance to reality since Apple invents their own version numbers.\n> \n> FWIW I was imagining just providing that -Wno-* on clang versions <= 11,\n> not special-casing Apple's in particular.\n\nIt's not about special-casing Apple in particular. It's that our\ndetect-compiler script does not understand which version of clang is\nused for Apple's compiler. Their version numbers are totally mismatched.\n\nSo you either have to say \"turn off this warning for clang totally\", or\ndo the wrong thing when Apple's compiler is in use.\n\n> I have a local patches that carry forward the idea I had in that thread,\n> i.e. to drop all this version detection insanity and just compile a C\n> program to detect the compiler.\n\nHmm. I prefer your earlier suggestion to use \"$(CC) -E\". This tool has\nto run on every invocation of \"make\", so the lighter-weight it is, the\nbetter. You say later...\n\n> The compilation is then triggered by the include in config.mak.dev,\n> which has a corresponding rule that creates the C program, then the\n> generated *.mak, so once we do it once we're only ever including an\n> already generated text file.\n\nbut that implies we don't have a dependency on the compiler itself.\nYou'd want to at least depend on the name of the compiler. But that\nwould also miss if running \"clang\" changes which version of clang you're\nrunning. I know upgrading your compiler is rare-ish, but this is exactly\nthe kind of thing I expect to bite at the most annoying time (when\nyou're switching around versions to try to figure out how they behave).\n\n> \t#ifdef __clang__\n> \t#if __clang_major__ >= 7\n> \t\tfn(util, \"NEEDS_std-eq-gnu99\", \"1\");\n> \t#endif\n\nThis is still gross version detection, but I don't think we can avoid\nit. However...\n\n> \t#if __has_warning(\"-Wextra\")\n> \t\tfn(util, \"HAS_Wextra\", \"1\");\n> \t#endif\n\n...this is much nicer. It could still be implemented purely via \"-E\", as\nfar as I can see, like:\n\n  #if __has_warning(\"-Wextra\")\n  HAS_Wextra = 1\n  #endif\n\nBut then we end up having to do version comparisons for gcc anyway:\n\n> \t#if __GNUC__ >= 6\n> \t\tfn(util, \"NEEDS_std-eq-gnu99\", \"1\");\n> \t\tfn(util, \"HAS_Wextra\", \"1\");\n\nso it feels like we're back where we started. You've just encoded the\nversion checks in a different spot.\n\nI dunno. I don't find this significantly less gross than the status quo.\nI don't mind getting the version via \"-E\" rather than \"-v\", but whether\nthe policy logic is in cpp, or in shell, or in the Makefile, it still\nneeds to exist. Putting it in cpp allows using has_warning(), but since\nthat isn't available everywhere, I'm not sure it buys us much.\n\n-Peff\n"},{"id":"459127","messageId":"220715.86edym230a.gmgdl@evledraar.gmail.com","threadId":"58127","inReplyTo":"YtCQl1oinrVnfa+6@coredump.intra.peff.net","subject":"Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-15T08:20:35Z","receivedAt":"2022-07-15T08:29:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 14 2022, Jeff King wrote:\n\n> On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >> I'm in favor of this. It would, of course, require extra\n>> >> special-casing for Apple's clang for which the version number bears no\n>> >> resemblance to reality since Apple invents their own version numbers.\n>> \n>> FWIW I was imagining just providing that -Wno-* on clang versions <= 11,\n>> not special-casing Apple's in particular.\n>\n> It's not about special-casing Apple in particular. It's that our\n> detect-compiler script does not understand which version of clang is\n> used for Apple's compiler. Their version numbers are totally mismatched.\n>\n> So you either have to say \"turn off this warning for clang totally\", or\n> do the wrong thing when Apple's compiler is in use.\n\nI missed the part where we don't even detect the version in that case,\nbut anyway, just turning it off on clang && OSX seems fine. This is\n-Wmissing-braces, we'll catch it elsewhere (CI etc.)\n\n>> I have a local patches that carry forward the idea I had in that thread,\n>> i.e. to drop all this version detection insanity and just compile a C\n>> program to detect the compiler.\n>\n> Hmm. I prefer your earlier suggestion to use \"$(CC) -E\". This tool has\n> to run on every invocation of \"make\", so the lighter-weight it is, the\n> better. You say later...\n\nWe only compile & run the binary once, then save the result to a *.mak\nfile, the intention is to get rid of running these binaries or script on\nevery single invocation.\n\n>> The compilation is then triggered by the include in config.mak.dev,\n>> which has a corresponding rule that creates the C program, then the\n>> generated *.mak, so once we do it once we're only ever including an\n>> already generated text file.\n>\n> but that implies we don't have a dependency on the compiler itself.\n> You'd want to at least depend on the name of the compiler. But that\n> would also miss if running \"clang\" changes which version of clang you're\n> running. I know upgrading your compiler is rare-ish, but this is exactly\n> the kind of thing I expect to bite at the most annoying time (when\n> you're switching around versions to try to figure out how they behave).\n\nThat's true, I considered that OK, and still do. I.e. we use this for\nconfig.mak, where we are opting you in to never flags.\n\nIf your compiler name changes your GIT-BUILD-OPTIONS will change, so\nwe'll re-build and re-detect.\n\nBut yes, if your CC=gcc and you change gcc from under us we won't\nre-detect and re-build.\n\nBut isn't that fine? You just won't get newer flags, you might downgrade\nyour gcc and get an error, but generally speaking make-based systems\nreally don't try to detect \"anything on the OS\" changed.\n\nStill, I could easily add something where we one-off run \"command -v\n$(CC)\", save that to a file, and then add that to our \"make\" dependency\ntree.\n\nSo then if the compiler binary's mtime changes we'd re-build, and we\nstill wouldn't run something on every invocation.\n\nIt just seems to be quite pedantic about correctness, is all :)\n\n>> \t#ifdef __clang__\n>> \t#if __clang_major__ >= 7\n>> \t\tfn(util, \"NEEDS_std-eq-gnu99\", \"1\");\n>> \t#endif\n>\n> This is still gross version detection, but I don't think we can avoid\n> it. However...\n\nYes, this part isn't really nicer, although with this method we could\nalso avoid this sort of thing by just trying the flag out & caching\nthat, but this seemed OK.\n\n>> \t#if __has_warning(\"-Wextra\")\n>> \t\tfn(util, \"HAS_Wextra\", \"1\");\n>> \t#endif\n>\n> ...this is much nicer. It could still be implemented purely via \"-E\", as\n> far as I can see, like:\n>\n>   #if __has_warning(\"-Wextra\")\n>   HAS_Wextra = 1\n>   #endif\n\n*nod*, I wish GCC had that. \n\n> But then we end up having to do version comparisons for gcc anyway:\n>\n>> \t#if __GNUC__ >= 6\n>> \t\tfn(util, \"NEEDS_std-eq-gnu99\", \"1\");\n>> \t\tfn(util, \"HAS_Wextra\", \"1\");\n>\n> so it feels like we're back where we started. You've just encoded the\n> version checks in a different spot.\n\nWe're really not, the reason I did this was because i tried to add\nsupport for xlc and suncc to this script, we don't even parse\ndetect-compiler on Solaris now, as its /bin/sh isn't compatible with\nit. So to begin with we'd need to hoist the \"shell detection and script\ngeneration\" out ...\n\n> I dunno. I don't find this significantly less gross than the status quo.\n> I don't mind getting the version via \"-E\" rather than \"-v\", but whether\n> the policy logic is in cpp, or in shell, or in the Makefile, it still\n> needs to exist. Putting it in cpp allows using has_warning(), but since\n> that isn't available everywhere, I'm not sure it buys us much.\n\nCompilers universally support \"who and what am I?\" via their native\nmacros, but we're trying to do it the really hard way via parsing\nversion output.\n"}]}