| Message ID | 1283182547-26116-11-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen<Jes.Sorensen@redhat.com> > > This keeps the compiler happy when building with -Wextra while > effectively generating the same code. > > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> > What's GCC's compliant? Regards, Anthony Liguori > --- > qjson.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/qjson.c b/qjson.c > index e4ee433..f259547 100644 > --- a/qjson.c > +++ b/qjson.c > @@ -36,8 +36,9 @@ static void parse_json(JSONMessageParser *parser, QList *tokens) > > QObject *qobject_from_jsonv(const char *string, va_list *ap) > { > - JSONParsingState state = {}; > + JSONParsingState state; > > + memset(&state, 0, sizeof(state)); > state.ap = ap; > > json_message_parser_init(&state.parser, parse_json); >
On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote: > - JSONParsingState state = {}; > + JSONParsingState state; > > + memset(&state, 0, sizeof(state)); > state.ap = ap; > JSONParsingState state = { .ap = ap }; achieves the same. Paolo
On 08/30/10 17:39, Anthony Liguori wrote: > On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen<Jes.Sorensen@redhat.com> >> >> This keeps the compiler happy when building with -Wextra while >> effectively generating the same code. >> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >> > > What's GCC's compliant? cc1: warnings being treated as errors qjson.c: In function 'qobject_from_jsonv': qjson.c:39: error: missing initializer qjson.c:39: error: (near initialization for 'state.parser') make: *** [qjson.o] Error 1 We have a lot of these where we try to init a struct element {}. Yes it's technically legal. However it's painful when you try to apply more aggressive warning flags looking for real bugs. I would suggest we modify the coding style to ask people to not init a struct like this. Cheers, Jes
On 08/30/2010 10:43 AM, Jes Sorensen wrote: > On 08/30/10 17:39, Anthony Liguori wrote: > >> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote: >> >>> From: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> This keeps the compiler happy when building with -Wextra while >>> effectively generating the same code. >>> >>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> >> What's GCC's compliant? >> > cc1: warnings being treated as errors > qjson.c: In function 'qobject_from_jsonv': > qjson.c:39: error: missing initializer > qjson.c:39: error: (near initialization for 'state.parser') > make: *** [qjson.o] Error 1 > > We have a lot of these where we try to init a struct element {}. > > Yes it's technically legal. However it's painful when you try to apply > more aggressive warning flags looking for real bugs. > No, this is GCC being stupid. > I would suggest we modify the coding style to ask people to not init a > struct like this. > How else do you terminate a list? IOW: MyDeviceInfo device_infos[] = { {"foo", 0, 2}, {"bar", 0, 1}, {} /* or { 0 } */ }; This is such a pervasive idiom that there's simply no way that GCC can possibly try to warn against this. Plus, it's entirely reasonable. I think this is just a false positive in GCC. Otherwise, there's a ton of code that it should be throwing warnings against. Regards, Anthony Liguori > Cheers, > Jes >
On 08/30/10 17:48, Anthony Liguori wrote: > On 08/30/2010 10:43 AM, Jes Sorensen wrote: >> Yes it's technically legal. However it's painful when you try to apply >> more aggressive warning flags looking for real bugs. > > No, this is GCC being stupid. > >> I would suggest we modify the coding style to ask people to not init a >> struct like this. >> > > How else do you terminate a list? IOW: > > MyDeviceInfo device_infos[] = { > {"foo", 0, 2}, > {"bar", 0, 1}, > {} /* or { 0 } */ > }; > > This is such a pervasive idiom that there's simply no way that GCC can > possibly try to warn against this. Plus, it's entirely reasonable. > > I think this is just a false positive in GCC. Otherwise, there's a ton > of code that it should be throwing warnings against I believe the comma after the last case takes care of terminating the list. I agree that it would be nice to get gcc to not moan about this specific case, however I will argue that my change is worth it to be able to use the error flags, even if it is gcc being stupid. Cheers, Jes
On 08/30/2010 10:42 AM, Paolo Bonzini wrote: > On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote: >> - JSONParsingState state = {}; >> + JSONParsingState state; >> >> + memset(&state, 0, sizeof(state)); >> state.ap = ap; >> > > JSONParsingState state = { .ap = ap }; > > achieves the same. But the fundamental is, what problem does GCC have with the original? If there isn't a reasonable answer, then I'm inclined to think this warning mode is a waste of time. Regards, Anthony Liguori > Paolo >
On 08/30/2010 06:15 PM, Anthony Liguori wrote: > On 08/30/2010 10:42 AM, Paolo Bonzini wrote: >> On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote: >>> - JSONParsingState state = {}; >>> + JSONParsingState state; >>> >>> + memset(&state, 0, sizeof(state)); >>> state.ap = ap; >>> >> >> JSONParsingState state = { .ap = ap }; >> >> achieves the same. > > But the fundamental is, what problem does GCC have with the original? If > there isn't a reasonable answer, then I'm inclined to think this warning > mode is a waste of time. It falls under the "missing fields in initializer" warning. Arguably, an empty initializer should be special cased, but it isn't. I agree that Jes's original patch is ugly, but the C99 initializer is an improvement. Paolo
On 08/30/2010 11:18 AM, Paolo Bonzini wrote: > On 08/30/2010 06:15 PM, Anthony Liguori wrote: >> On 08/30/2010 10:42 AM, Paolo Bonzini wrote: >>> On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote: >>>> - JSONParsingState state = {}; >>>> + JSONParsingState state; >>>> >>>> + memset(&state, 0, sizeof(state)); >>>> state.ap = ap; >>>> >>> >>> JSONParsingState state = { .ap = ap }; >>> >>> achieves the same. >> >> But the fundamental is, what problem does GCC have with the original? If >> there isn't a reasonable answer, then I'm inclined to think this warning >> mode is a waste of time. > > It falls under the "missing fields in initializer" warning. Arguably, > an empty initializer should be special cased, but it isn't. So the warning is for old style initializer lists? I disagree that it's a valid warning. First, {} is ambiguous as it can be an empty list of c99 initializers and an empty list of c89 initializers. But even for c89 initializers, it's very common practice to omit initializers and rely on the defaulted value. For instance, { 0 } is quite pervasive as an idiom. > > I agree that Jes's original patch is ugly, but the C99 initializer is > an improvement. Yes, I'm fine with your patch on it's own but I disagree with GCC's warnings here. Regards, Anthony Liguori > Paolo
On 08/30/2010 06:29 PM, Anthony Liguori wrote: >> Arguably, an empty initializer should be special cased, but it >> isn't. > > So the warning is for old style initializer lists? I disagree that > it's a valid warning. First, {} is ambiguous as it can be an empty > list of c99 initializers and an empty list of c89 initializers. Yes, that's what I meant. {} should be special cased as an empty list will rarely be a bug, even when C89 initializers are used. > But even for c89 initializers, it's very common practice to omit > initializers and rely on the defaulted value. For instance, { 0 } is > quite pervasive as an idiom. That's why the warning is only in -Wextra. Those are warnings that may clash with someone's style conventions. Paolo
On Mon, Aug 30, 2010 at 10:48:55AM -0500, Anthony Liguori wrote: > No, this is GCC being stupid. > > How else do you terminate a list? IOW: > > MyDeviceInfo device_infos[] = { > {"foo", 0, 2}, > {"bar", 0, 1}, > {} /* or { 0 } */ > }; > > This is such a pervasive idiom that there's simply no way that GCC can > possibly try to warn against this. Plus, it's entirely reasonable. > > I think this is just a false positive in GCC. Otherwise, there's a ton > of code that it should be throwing warnings against. Well, it sounds like Jes was compiling QEMU was extra warning flags, and I doubt people do much beyond -Wall and maybe one or two others. I could see petitioning GCC to only complain if -pedantic. -Nathan
On Mon, 30 Aug 2010, Anthony Liguori wrote: > On 08/30/2010 10:43 AM, Jes Sorensen wrote: > > On 08/30/10 17:39, Anthony Liguori wrote: > > > > > On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote: > > > > > > > From: Jes Sorensen<Jes.Sorensen@redhat.com> > > > > > > > > This keeps the compiler happy when building with -Wextra while > > > > effectively generating the same code. > > > > > > > > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> > > > > > > > > > > > What's GCC's compliant? > > > > > cc1: warnings being treated as errors > > qjson.c: In function 'qobject_from_jsonv': > > qjson.c:39: error: missing initializer > > qjson.c:39: error: (near initialization for 'state.parser') > > make: *** [qjson.o] Error 1 > > > > We have a lot of these where we try to init a struct element {}. > > > > Yes it's technically legal. However it's painful when you try to apply > > more aggressive warning flags looking for real bugs. > > > > No, this is GCC being stupid. Nonsense, it would have been stupid if it warned without asking for this warning, this is GCC being intelligent. [..snip..]
On 08/30/2010 06:53 PM, Jes Sorensen wrote: > On 08/30/10 17:48, Anthony Liguori wrote: >> On 08/30/2010 10:43 AM, Jes Sorensen wrote: >>> Yes it's technically legal. However it's painful when you try to apply >>> more aggressive warning flags looking for real bugs. >> No, this is GCC being stupid. >> >>> I would suggest we modify the coding style to ask people to not init a >>> struct like this. >>> >> How else do you terminate a list? IOW: >> >> MyDeviceInfo device_infos[] = { >> {"foo", 0, 2}, >> {"bar", 0, 1}, >> {} /* or { 0 } */ >> }; >> >> This is such a pervasive idiom that there's simply no way that GCC can >> possibly try to warn against this. Plus, it's entirely reasonable. >> >> I think this is just a false positive in GCC. Otherwise, there's a ton >> of code that it should be throwing warnings against > I believe the comma after the last case takes care of terminating the list. It only makes patches that add items prettier. > I agree that it would be nice to get gcc to not moan about this specific > case, however I will argue that my change is worth it to be able to use > the error flags, even if it is gcc being stupid. If the flags make gcc stupid, why use them?
On 08/30/10 18:55, Nathan Froyd wrote: > On Mon, Aug 30, 2010 at 10:48:55AM -0500, Anthony Liguori wrote: >> No, this is GCC being stupid. >> >> How else do you terminate a list? IOW: >> >> MyDeviceInfo device_infos[] = { >> {"foo", 0, 2}, >> {"bar", 0, 1}, >> {} /* or { 0 } */ >> }; >> >> This is such a pervasive idiom that there's simply no way that GCC can >> possibly try to warn against this. Plus, it's entirely reasonable. >> >> I think this is just a false positive in GCC. Otherwise, there's a ton >> of code that it should be throwing warnings against. > > Well, it sounds like Jes was compiling QEMU was extra warning flags, and > I doubt people do much beyond -Wall and maybe one or two others. I was! I am not arguing that we should always compile QEMU with such aggressive flags, but I my search did turn up several genuine bugs. What I do argue in favor of is when it makes sense to make small IMHO harmless changes to the code, that makes it easier to build QEMU with aggressive C flags in order to catch real bugs. Cheers, Jes
On 08/30/10 18:29, Anthony Liguori wrote: > On 08/30/2010 11:18 AM, Paolo Bonzini wrote: >> It falls under the "missing fields in initializer" warning. Arguably, >> an empty initializer should be special cased, but it isn't. > > So the warning is for old style initializer lists? I disagree that it's > a valid warning. First, {} is ambiguous as it can be an empty list of > c99 initializers and an empty list of c89 initializers. > > But even for c89 initializers, it's very common practice to omit > initializers and rely on the defaulted value. For instance, { 0 } is > quite pervasive as an idiom. > >> >> I agree that Jes's original patch is ugly, but the C99 initializer is >> an improvement. > > Yes, I'm fine with your patch on it's own but I disagree with GCC's > warnings here. So if we pulled in Paolo's version instead, would you be happy with that? Personally I find the memset approach prettier since it's explicit what it does, but that is obviously down to personal preference. Cheers, Jes
On 08/30/2010 08:48 AM, Anthony Liguori wrote: > MyDeviceInfo device_infos[] = { {"foo", 0, 2}, {"bar", 0, 1}, {} /* > or { 0 } */ }; > > This is such a pervasive idiom that there's simply no way that GCC > can possibly try to warn against this. GCC only warns for this if you explicitly ask for it. I.e. -Wmissing-field-initializers. That flag is included in -Wextra, which Jes used. Traditionally this flag is used when you are initializing structures like the above, but you make a change to the structure involved, so you may need to update all initializers to take the new field into account. Some projects have found this idiom useful. That said, today I'd not bother with this flag and use designated initializers now. r~
diff --git a/qjson.c b/qjson.c index e4ee433..f259547 100644 --- a/qjson.c +++ b/qjson.c @@ -36,8 +36,9 @@ static void parse_json(JSONMessageParser *parser, QList *tokens) QObject *qobject_from_jsonv(const char *string, va_list *ap) { - JSONParsingState state = {}; + JSONParsingState state; + memset(&state, 0, sizeof(state)); state.ap = ap; json_message_parser_init(&state.parser, parse_json);