* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
@ 2013-03-03 22:09 cgit
2013-03-03 22:16 ` dpmcgee
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: cgit @ 2013-03-03 22:09 UTC (permalink / raw)
Explicitly set the suffix field of the terminating format entry to 0.
This fixes a GCC warning seen with "-Wmissing-field-initializers".
Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
ui-snapshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ui-snapshot.c b/ui-snapshot.c
index 47432bd..e740645 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
{ ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
{ ".tar", "application/x-tar", write_tar_archive, 0x08 },
{ ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
- {}
+ { 0 }
};
static const struct cgit_snapshot_format *get_format(const char *filename)
--
1.8.2.rc0.247.g811e0c0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 22:09 [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly cgit
@ 2013-03-03 22:16 ` dpmcgee
2013-03-04 7:01 ` cgit
2013-03-03 22:55 ` john
2013-03-03 23:51 ` [PATCH v2] " cgit
2 siblings, 1 reply; 9+ messages in thread
From: dpmcgee @ 2013-03-03 22:16 UTC (permalink / raw)
On Sun, Mar 3, 2013 at 4:09 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> Explicitly set the suffix field of the terminating format entry to 0.
> This fixes a GCC warning seen with "-Wmissing-field-initializers".
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
> ui-snapshot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui-snapshot.c b/ui-snapshot.c
> index 47432bd..e740645 100644
> --- a/ui-snapshot.c
> +++ b/ui-snapshot.c
> @@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
> { ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
> { ".tar", "application/x-tar", write_tar_archive, 0x08 },
> { ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
> - {}
> + { 0 }
Why 0 and not NULL here, as is convention for strings?
> };
>
> static const struct cgit_snapshot_format *get_format(const char *filename)
> --
> 1.8.2.rc0.247.g811e0c0
>
>
> _______________________________________________
> cgit mailing list
> cgit at hjemli.net
> http://hjemli.net/mailman/listinfo/cgit
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 22:09 [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly cgit
2013-03-03 22:16 ` dpmcgee
@ 2013-03-03 22:55 ` john
2013-03-03 23:41 ` cgit
2013-03-03 23:51 ` [PATCH v2] " cgit
2 siblings, 1 reply; 9+ messages in thread
From: john @ 2013-03-03 22:55 UTC (permalink / raw)
On Sun, Mar 03, 2013 at 11:09:01PM +0100, Lukas Fleischer wrote:
> Explicitly set the suffix field of the terminating format entry to 0.
> This fixes a GCC warning seen with "-Wmissing-field-initializers".
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
> ui-snapshot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui-snapshot.c b/ui-snapshot.c
> index 47432bd..e740645 100644
> --- a/ui-snapshot.c
> +++ b/ui-snapshot.c
> @@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
> { ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
> { ".tar", "application/x-tar", write_tar_archive, 0x08 },
> { ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
> - {}
> + { 0 }
I'm mildly against this - we're not fixing an issue that's been found with some
specific compiler, the meaning of both versions is the same and
-Wmissing-field-initializers isn't in -Wall. It feels like a warning that was
added for people following some overly prescriptive coding standard, not a
warning that's actually useful.
[Also, I'm surprised this is sufficient to squelch the warning given the
description of -Wmissing-field-initializers in gcc(1):
Warn if a structure's initializer has some fields missing. For example,
the following code would cause such a warning, because "x.h" is implicitly
zero:
struct s { int f, g, h; };
struct s x = { 3, 4 };
]
> };
>
> static const struct cgit_snapshot_format *get_format(const char *filename)
> --
> 1.8.2.rc0.247.g811e0c0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 22:55 ` john
@ 2013-03-03 23:41 ` cgit
2013-03-03 23:53 ` john
2013-03-03 23:59 ` Jason
0 siblings, 2 replies; 9+ messages in thread
From: cgit @ 2013-03-03 23:41 UTC (permalink / raw)
On Sun, Mar 03, 2013 at 10:55:16PM +0000, John Keeping wrote:
> On Sun, Mar 03, 2013 at 11:09:01PM +0100, Lukas Fleischer wrote:
> > Explicitly set the suffix field of the terminating format entry to 0.
> > This fixes a GCC warning seen with "-Wmissing-field-initializers".
> >
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> > ui-snapshot.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ui-snapshot.c b/ui-snapshot.c
> > index 47432bd..e740645 100644
> > --- a/ui-snapshot.c
> > +++ b/ui-snapshot.c
> > @@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
> > { ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
> > { ".tar", "application/x-tar", write_tar_archive, 0x08 },
> > { ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
> > - {}
> > + { 0 }
>
> I'm mildly against this - we're not fixing an issue that's been found with some
> specific compiler, the meaning of both versions is the same and
> -Wmissing-field-initializers isn't in -Wall. It feels like a warning that was
> added for people following some overly prescriptive coding standard, not a
> warning that's actually useful.
Looking at the C89/C99 standards (section 6.7.8), it doesn't look like
using "{}" as initializer is valid at all. In the C++ standard, a
special grammar rule was added to allow for using "{}" as initializer
(see section 8.5). I guess we're rather fixing a "real" bug (nothing of
practical importance, but definitely not a compiler-related issue) here.
Maybe the commit message should be updated to reflect this.
>
> [Also, I'm surprised this is sufficient to squelch the warning given the
> description of -Wmissing-field-initializers in gcc(1):
>
> Warn if a structure's initializer has some fields missing. For example,
> the following code would cause such a warning, because "x.h" is implicitly
> zero:
>
> struct s { int f, g, h; };
> struct s x = { 3, 4 };
> ]
Yes, it's a bit weird. My guess is that if you're doing something like
"struct s x = { 3, 4 };" in the example above, you most likely forgot to
set the last field, whereas if you're doing something like "struct s x =
{ 0 };" you're explicitly (and deliberately) initializing everything to
zeros. The compiler is kind of guessing whether the missing members are
skipped on purpose or not.
>
> > };
> >
> > static const struct cgit_snapshot_format *get_format(const char *filename)
> > --
> > 1.8.2.rc0.247.g811e0c0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 22:09 [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly cgit
2013-03-03 22:16 ` dpmcgee
2013-03-03 22:55 ` john
@ 2013-03-03 23:51 ` cgit
2 siblings, 0 replies; 9+ messages in thread
From: cgit @ 2013-03-03 23:51 UTC (permalink / raw)
According to section 6.7.8 of the C89/C99 standards, initializer lists
must not be empty. Explicitly set the first field to zero which is the
standard way of initializing a structure to all zeros.
This also fixes a GCC warning seen with "-Wmissing-field-initializers".
Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
Add a better commit message, see discussion on the original patch.
ui-snapshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ui-snapshot.c b/ui-snapshot.c
index 47432bd..e740645 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
{ ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
{ ".tar", "application/x-tar", write_tar_archive, 0x08 },
{ ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
- {}
+ { 0 }
};
static const struct cgit_snapshot_format *get_format(const char *filename)
--
1.8.2.rc0.247.g811e0c0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 23:41 ` cgit
@ 2013-03-03 23:53 ` john
2013-03-03 23:59 ` Jason
1 sibling, 0 replies; 9+ messages in thread
From: john @ 2013-03-03 23:53 UTC (permalink / raw)
On Mon, Mar 04, 2013 at 12:41:18AM +0100, Lukas Fleischer wrote:
> On Sun, Mar 03, 2013 at 10:55:16PM +0000, John Keeping wrote:
> > On Sun, Mar 03, 2013 at 11:09:01PM +0100, Lukas Fleischer wrote:
> > > Explicitly set the suffix field of the terminating format entry to 0.
> > > This fixes a GCC warning seen with "-Wmissing-field-initializers".
> > >
> > > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > > ---
> > > ui-snapshot.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/ui-snapshot.c b/ui-snapshot.c
> > > index 47432bd..e740645 100644
> > > --- a/ui-snapshot.c
> > > +++ b/ui-snapshot.c
> > > @@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
> > > { ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
> > > { ".tar", "application/x-tar", write_tar_archive, 0x08 },
> > > { ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
> > > - {}
> > > + { 0 }
> >
> > I'm mildly against this - we're not fixing an issue that's been found with some
> > specific compiler, the meaning of both versions is the same and
> > -Wmissing-field-initializers isn't in -Wall. It feels like a warning that was
> > added for people following some overly prescriptive coding standard, not a
> > warning that's actually useful.
>
> Looking at the C89/C99 standards (section 6.7.8), it doesn't look like
> using "{}" as initializer is valid at all. In the C++ standard, a
> special grammar rule was added to allow for using "{}" as initializer
> (see section 8.5). I guess we're rather fixing a "real" bug (nothing of
> practical importance, but definitely not a compiler-related issue) here.
> Maybe the commit message should be updated to reflect this.
Interesting. A careful reading agrees with this, although I haven't
come across a compiler that would treat these differently and it's
slightly strange that you can't keep reducing the "unspecified members
take a default value" case until the list is empty.
> >
> > [Also, I'm surprised this is sufficient to squelch the warning given the
> > description of -Wmissing-field-initializers in gcc(1):
> >
> > Warn if a structure's initializer has some fields missing. For example,
> > the following code would cause such a warning, because "x.h" is implicitly
> > zero:
> >
> > struct s { int f, g, h; };
> > struct s x = { 3, 4 };
> > ]
>
> Yes, it's a bit weird. My guess is that if you're doing something like
> "struct s x = { 3, 4 };" in the example above, you most likely forgot to
> set the last field, whereas if you're doing something like "struct s x =
> { 0 };" you're explicitly (and deliberately) initializing everything to
> zeros. The compiler is kind of guessing whether the missing members are
> skipped on purpose or not.
>
> >
> > > };
> > >
> > > static const struct cgit_snapshot_format *get_format(const char *filename)
> > > --
> > > 1.8.2.rc0.247.g811e0c0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 23:41 ` cgit
2013-03-03 23:53 ` john
@ 2013-03-03 23:59 ` Jason
1 sibling, 0 replies; 9+ messages in thread
From: Jason @ 2013-03-03 23:59 UTC (permalink / raw)
John is right here -- { } isn't valid C -- GCC is fine with it though.
{ 0 } or { NULL } are both extremely widespread terminators, so I
could imagine gcc special casing it for Wmissing-field-initializers.
Furthermore, when you do neglect to set all the fields explicitly, the
unset ones are zeroed out. So, the zero in { 0 } doesn't ever really
apply to any particular field, since they're all set to that value.
It's basically a nice syntax for zeroing stack variables.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-03 22:16 ` dpmcgee
@ 2013-03-04 7:01 ` cgit
2013-03-04 14:13 ` Jason
0 siblings, 1 reply; 9+ messages in thread
From: cgit @ 2013-03-04 7:01 UTC (permalink / raw)
On Sun, Mar 03, 2013 at 04:16:35PM -0600, Dan McGee wrote:
> On Sun, Mar 3, 2013 at 4:09 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> > Explicitly set the suffix field of the terminating format entry to 0.
> > This fixes a GCC warning seen with "-Wmissing-field-initializers".
> >
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> > ui-snapshot.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ui-snapshot.c b/ui-snapshot.c
> > index 47432bd..e740645 100644
> > --- a/ui-snapshot.c
> > +++ b/ui-snapshot.c
> > @@ -48,7 +48,7 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
> > { ".tar.bz2", "application/x-bzip2", write_tar_bzip2_archive, 0x04 },
> > { ".tar", "application/x-tar", write_tar_archive, 0x08 },
> > { ".tar.xz", "application/x-xz", write_tar_xz_archive, 0x10 },
> > - {}
> > + { 0 }
> Why 0 and not NULL here, as is convention for strings?
I agree that "NULL" is a slightly better choice here.
Jason: Could you please fix this before merging this patch?
>
> > };
> >
> > static const struct cgit_snapshot_format *get_format(const char *filename)
> > --
> > 1.8.2.rc0.247.g811e0c0
> >
> >
> > _______________________________________________
> > cgit mailing list
> > cgit at hjemli.net
> > http://hjemli.net/mailman/listinfo/cgit
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly
2013-03-04 7:01 ` cgit
@ 2013-03-04 14:13 ` Jason
0 siblings, 0 replies; 9+ messages in thread
From: Jason @ 2013-03-04 14:13 UTC (permalink / raw)
On Mon, Mar 4, 2013 at 2:01 AM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> I agree that "NULL" is a slightly better choice here.
>
> Jason: Could you please fix this before merging this patch?
Done.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-03-04 14:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-03 22:09 [PATCH] ui-snapshot.c: Terminate cgit_snapshot_formats[] properly cgit
2013-03-03 22:16 ` dpmcgee
2013-03-04 7:01 ` cgit
2013-03-04 14:13 ` Jason
2013-03-03 22:55 ` john
2013-03-03 23:41 ` cgit
2013-03-03 23:53 ` john
2013-03-03 23:59 ` Jason
2013-03-03 23:51 ` [PATCH v2] " cgit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).