Patchwork [10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

login
register
mail settings
Submitter Jes Sorensen
Date Aug. 30, 2010, 3:35 p.m.
Message ID <1283182547-26116-11-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/63051/
State New
Headers show

Comments

Jes Sorensen - Aug. 30, 2010, 3:35 p.m.
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>
---
 qjson.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Anthony Liguori - Aug. 30, 2010, 3:39 p.m.
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);
>
Paolo Bonzini - Aug. 30, 2010, 3:42 p.m.
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
Jes Sorensen - Aug. 30, 2010, 3:43 p.m.
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
Anthony Liguori - Aug. 30, 2010, 3:48 p.m.
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
>
Jes Sorensen - Aug. 30, 2010, 3:53 p.m.
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
Anthony Liguori - Aug. 30, 2010, 4:15 p.m.
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
>
Paolo Bonzini - Aug. 30, 2010, 4:18 p.m.
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
Anthony Liguori - Aug. 30, 2010, 4:29 p.m.
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
Paolo Bonzini - Aug. 30, 2010, 4:35 p.m.
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
Nathan Froyd - Aug. 30, 2010, 4:55 p.m.
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
malc - Aug. 30, 2010, 5:06 p.m.
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..]
Avi Kivity - Aug. 30, 2010, 5:12 p.m.
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?
Jes Sorensen - Aug. 30, 2010, 6 p.m.
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
Jes Sorensen - Aug. 30, 2010, 6:05 p.m.
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
Richard Henderson - Aug. 30, 2010, 7:32 p.m.
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~

Patch

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);