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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 12, 2022, 07:13 UTC
Message-ID: <CAPig+cSgNB=SzAZLhXvteSYmy0HvJh+qWHMYyBxcX_EA9__u4A@mail.gmail.com>
In-Reply-To: <220712.86lesy6cri.gmgdl@evledraar.gmail.com>

On Tue, Jul 12, 2022 at 2:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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