[PATCH] merge-file: fix build warning with gcc 4.8.5

[PATCH] merge-file: fix build warning with gcc 4.8.5

[Đoàn Trần Công Danh]

mmfs is an array of struct, xmparamt_t's first field is a struct,
mmfs's element and xmparamt_t's first field must be initialised
with {0}.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

 This warning is available in master

 builtin/merge-file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index c923bbf2ab..607c3d3f9e 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -26,9 +26,9 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {
 	const char *names[3] = { 0 };
-	mmfile_t mmfs[3] = { 0 };
+	mmfile_t mmfs[3] = { { 0 } };
 	mmbuffer_t result = { 0 };
-	xmparam_t xmp = { 0 };
+	xmparam_t xmp = { { 0 } };
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
 	struct option options[] = {
-- 
2.37.1.560.gdfb9273964

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> mmfs is an array of struct, xmparamt_t's first field is a struct,
> mmfs's element and xmparamt_t's first field must be initialised
> with {0}.
>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>
>  This warning is available in master

Thanks, but didn't we discuss this recently and decided that such
(false) complaints by ancient compiler is not worth catering to by
deliberately deviating the "zero initializing a struct is always
done via { 0 }" idiom?

cf. https://lore.kernel.org/git/CAPig+cSgNB=SzAZLhXvteSYmy0HvJh+qWHMYyBxcX_EA9__u4A@mail.gmail.com/

I think the concensus was that we should squelch the false warning
on older compilers with -Wno-missing-braces, but then the discussion
has stalled by a suggestion to introduce a way to detect older
compilers that is different from how we do so at the same time, and
went nowhere.

Hopefully we can add a simple -Wno-* without waiting for whole
config.mak thing getting revamped this time?

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Jeff King]

On Fri, Jul 29, 2022 at 08:48:46AM -0700, Junio C Hamano wrote:

> I think the concensus was that we should squelch the false warning
> on older compilers with -Wno-missing-braces, but then the discussion
> has stalled by a suggestion to introduce a way to detect older
> compilers that is different from how we do so at the same time, and
> went nowhere.
> 
> Hopefully we can add a simple -Wno-* without waiting for whole
> config.mak thing getting revamped this time?

Perhaps this?

-- >8 --
Subject: [PATCH] config.mak.dev: squelch -Wno-missing-braces for older gcc

Versions of gcc prior to 4.9 complain about an initialization like:

  struct inner { int x; };
  struct outer { struct inner; };
  struct outer foo = { 0 };

and insist on:

  struct outer foo = { { 0 } };

Newer compilers handle this just fine. And ignoring the window even on
older compilers is fine; the resulting code is correct, but we just get
caught by -Werror.

Let's relax this for older compilers to make developer lives easier (we
don't care much about non-developers on old compilers; they may see a
warning, but it won't stop compilation).

Signed-off-by: Jeff King <peff@peff.net>
---
Tested on a debian jessie chroot using gcc-4.8 and 4.9. Though note that
you also need to manually specify -std=gnu99 to get it to work at all
with those compilers these days! So I kind of wonder if it's even worth
catering to their warnings automatically.

 config.mak.dev | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/config.mak.dev b/config.mak.dev
index 335efd4620..b9878a4994 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -59,9 +59,13 @@ endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining
+#
+# Likwise, gcc older than 4.9 complains about initializing a
+# struct-within-a-struct using just "{ 0 }"
 ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
 ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-uninitialized
+DEVELOPER_CFLAGS += -Wno-missing-braces
 endif
 endif
 
-- 
2.37.1.804.g1775fa20e0

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Jeff King]

On Fri, Jul 29, 2022 at 03:53:54PM -0400, Jeff King wrote:

>  # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
>  # not worth fixing since newer compilers correctly stop complaining
> +#
> +# Likwise, gcc older than 4.9 complains about initializing a
> +# struct-within-a-struct using just "{ 0 }"

s/Lik/Like/

That's what I get for last-minute editing. :)

-Peff

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Eric Sunshine]

On Fri, Jul 29, 2022 at 3:53 PM Jeff King <peff@peff.net> wrote:
> Subject: [PATCH] config.mak.dev: squelch -Wno-missing-braces for older gcc
>
> Versions of gcc prior to 4.9 complain about an initialization like:
>
>   struct inner { int x; };
>   struct outer { struct inner; };
>   struct outer foo = { 0 };
>
> and insist on:
>
>   struct outer foo = { { 0 } };
>
> Newer compilers handle this just fine. And ignoring the window even on
> older compilers is fine; the resulting code is correct, but we just get
> caught by -Werror.
>
> Let's relax this for older compilers to make developer lives easier (we
> don't care much about non-developers on old compilers; they may see a
> warning, but it won't stop compilation).
>
> Signed-off-by: Jeff King <peff@peff.net>

Leaving aside for the moment the problem with Apple's oddball invented
version numbers for `clang`, should this patch also take older `clang`
versions into consideration rather than focusing only on `gcc`? (Of
course, `clang` could be dealt with in a separate patch if you'd
rather not worry about it here.)

> diff --git a/config.mak.dev b/config.mak.dev
> index 335efd4620..b9878a4994 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -59,9 +59,13 @@ endif
>
>  # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
>  # not worth fixing since newer compilers correctly stop complaining
> +#
> +# Likwise, gcc older than 4.9 complains about initializing a
> +# struct-within-a-struct using just "{ 0 }"
>  ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
>  ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
>  DEVELOPER_CFLAGS += -Wno-uninitialized
> +DEVELOPER_CFLAGS += -Wno-missing-braces
>  endif
>  endif

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Jeff King]

On Fri, Jul 29, 2022 at 04:48:55PM -0400, Eric Sunshine wrote:

> Leaving aside for the moment the problem with Apple's oddball invented
> version numbers for `clang`, should this patch also take older `clang`
> versions into consideration rather than focusing only on `gcc`? (Of
> course, `clang` could be dealt with in a separate patch if you'd
> rather not worry about it here.)

I was just fixing the reported gcc issue, and forgot totally that clang
had been mentioned in previous rounds. I'd be happy to just see a clang
patch on top of this once somebody figures out the right versions (but
it may be impossible without figuring out the oddball Apple thing).

-Peff

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 29, 2022 at 04:48:55PM -0400, Eric Sunshine wrote:
>
>> Leaving aside for the moment the problem with Apple's oddball invented
>> version numbers for `clang`, should this patch also take older `clang`
>> versions into consideration rather than focusing only on `gcc`? (Of
>> course, `clang` could be dealt with in a separate patch if you'd
>> rather not worry about it here.)
>
> I was just fixing the reported gcc issue, and forgot totally that clang
> had been mentioned in previous rounds. I'd be happy to just see a clang
> patch on top of this once somebody figures out the right versions (but
> it may be impossible without figuring out the oddball Apple thing).

I am willing to say that we do not care about "oddball Apple thing"
and have developers on that platform to propose how to handle their
compiler.

In any case, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
seems to indicate that given enough time this problem will
disappear, so I'd refrain from suggesting to use -Wno-missing-braces
everywhere.

Thanks.

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Đoàn Trần Công Danh]

On 2022-07-29 15:53:53-0400, Jeff King <peff@peff.net> wrote:
> On Fri, Jul 29, 2022 at 08:48:46AM -0700, Junio C Hamano wrote:
> 
> > I think the concensus was that we should squelch the false warning
> > on older compilers with -Wno-missing-braces, but then the discussion
> > has stalled by a suggestion to introduce a way to detect older
> > compilers that is different from how we do so at the same time, and
> > went nowhere.
> > 
> > Hopefully we can add a simple -Wno-* without waiting for whole
> > config.mak thing getting revamped this time?
> 
> Perhaps this?
> 
> -- >8 --
> Subject: [PATCH] config.mak.dev: squelch -Wno-missing-braces for older gcc
> 
> Versions of gcc prior to 4.9 complain about an initialization like:
> 
>   struct inner { int x; };
>   struct outer { struct inner; };
>   struct outer foo = { 0 };
> 
> and insist on:
> 
>   struct outer foo = { { 0 } };
> 
> Newer compilers handle this just fine. And ignoring the window even on
> older compilers is fine; the resulting code is correct, but we just get
> caught by -Werror.
> 
> Let's relax this for older compilers to make developer lives easier (we
> don't care much about non-developers on old compilers; they may see a
> warning, but it won't stop compilation).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Tested on a debian jessie chroot using gcc-4.8 and 4.9. Though note that
> you also need to manually specify -std=gnu99 to get it to work at all
> with those compilers these days! So I kind of wonder if it's even worth
> catering to their warnings automatically.

Well, config.mak.uname automatically adds -std=c99 for RHEL 7 and
CentOS7. Can we add the same things for Debian? Or should we just
remove both?


-- 
Danh

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Jeff King]

On Sat, Jul 30, 2022 at 07:19:49AM +0700, Đoàn Trần Công Danh wrote:

> > Tested on a debian jessie chroot using gcc-4.8 and 4.9. Though note that
> > you also need to manually specify -std=gnu99 to get it to work at all
> > with those compilers these days! So I kind of wonder if it's even worth
> > catering to their warnings automatically.
> 
> Well, config.mak.uname automatically adds -std=c99 for RHEL 7 and
> CentOS7. Can we add the same things for Debian? Or should we just
> remove both?

Ah, I didn't know about that. No, I don't think there's any reason to
remove them. If people are able to compile out of the box there, then my
patch to config.mak.dev may be worth doing.

As far as adding a similar config.mak.uname thing for Debian, I don't
have a strong opinion. Jessie is far beyond being supported, so I'd
probably wait to see if somebody who actually cares proposes a patch.

-Peff

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On 2022-07-30 at 00:19:49, Đoàn Trần Công Danh wrote:
> On 2022-07-29 15:53:53-0400, Jeff King <peff@peff.net> wrote:
> > Tested on a debian jessie chroot using gcc-4.8 and 4.9. Though note that
> > you also need to manually specify -std=gnu99 to get it to work at all
> > with those compilers these days! So I kind of wonder if it's even worth
> > catering to their warnings automatically.
> 
> Well, config.mak.uname automatically adds -std=c99 for RHEL 7 and
> CentOS7. Can we add the same things for Debian? Or should we just
> remove both?

I don't think we can do that, since Debian kernels don't include a
distinguishing pattern like that[0].  Also, Debian jessie doesn't have a
full set of security support, unlike CentOS 7, and thus I would argue we
probably wouldn't want to support it anyway.

My guess is that Peff used jessie because he uses Debian and it's easier
for him to set up than CentOS 7, not because we should use it as an
intentional target.

Personally, although I don't use RHEL and company in either my personal
or professional life anymore, I think it's probably worth providing a
modicum of support to because they're very common, at least as long as
there are freely available clones with security support (e.g., CentOS
and Rocky Linux) that we can test against.

All that to say that I think we don't need to change config.mak.uname
and can rely on folks just setting -std=c99 if need be.

[0] Okay, there is a pattern, but it doesn't include a fixed string and
neither shell nor make are ideal for pattern matching on it.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] merge-file: fix build warning with gcc 4.8.5

[Jeff King]

On Sat, Jul 30, 2022 at 01:46:46AM +0000, brian m. carlson wrote:

> > Well, config.mak.uname automatically adds -std=c99 for RHEL 7 and
> > CentOS7. Can we add the same things for Debian? Or should we just
> > remove both?
> 
> I don't think we can do that, since Debian kernels don't include a
> distinguishing pattern like that[0].  Also, Debian jessie doesn't have a
> full set of security support, unlike CentOS 7, and thus I would argue we
> probably wouldn't want to support it anyway.

The check really ought to be using the compiler version and not uname
anyway. But outside of DEVELOPER=1, we don't (yet) do any probing of the
compiler.

> My guess is that Peff used jessie because he uses Debian and it's easier
> for him to set up than CentOS 7, not because we should use it as an
> intentional target.

Yep, exactly.

> Personally, although I don't use RHEL and company in either my personal
> or professional life anymore, I think it's probably worth providing a
> modicum of support to because they're very common, at least as long as
> there are freely available clones with security support (e.g., CentOS
> and Rocky Linux) that we can test against.
> 
> All that to say that I think we don't need to change config.mak.uname
> and can rely on folks just setting -std=c99 if need be.

Agreed. I only brought it up as a "gee, is anybody even using this?"
head-scratcher. I just wasn't aware of the CentOS workaround.

-Peff