| Message ID | 365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge-file: fix build warning with gcc 4.8.5 | expand |
Đ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?
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
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
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
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
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.
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?
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
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.
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
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[] = {
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(-)