* [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: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
* [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] 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 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
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).