diff mbox

configure.ac: disable annoying warning -Wmissing-field-initializers

Message ID 1452633260-26767-1-git-send-email-maraeo@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marek Olšák Jan. 12, 2016, 9:14 p.m. UTC
From: Marek Olšák <marek.olsak@amd.com>

It warns for all "{}" initializers. Well, I want us to use {}.
---
 configure.ac         | 3 ++-
 intel/intel_decode.c | 2 --
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Emil Velikov Jan. 15, 2016, 11:12 a.m. UTC | #1
On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> It warns for all "{}" initializers. Well, I want us to use {}.
> ---
>  configure.ac         | 3 ++-
>  intel/intel_decode.c | 2 --
The whole of libdrm, minus the intel_decode can get away without using
such constructs. And yes that includes radeon and amdgpu.

NACK on this one - please be consistent with existing code base.

-Emil
Marek Olšák Jan. 15, 2016, 3:24 p.m. UTC | #2
On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> It warns for all "{}" initializers. Well, I want us to use {}.
>> ---
>>  configure.ac         | 3 ++-
>>  intel/intel_decode.c | 2 --
> The whole of libdrm, minus the intel_decode can get away without using
> such constructs. And yes that includes radeon and amdgpu.
>
> NACK on this one - please be consistent with existing code base.

Consistent with what? {} is the same as memset on each structure
member. The warning says that a structure member is initialized to
zero because of {}, which is why {} is used in the first place. It's
the same as using memset and getting a warning "memset initializes the
memory to zero". How useful is that?

libdrm does have a lot of optional warnings enabled. Mesa does not,
and Mesa does not even have this one. This means libdrm is
inconsistent with Mesa and, BTW, it's also inconsistent with the kernel.

It looks like somebody enabled optional warnings for libdrm in the
past. All I'm doing is aligning the behavior with Mesa/kernel, which
is what we would like to have and so would Intel apparently.

Do you still think we are inconsistent?

Thanks,

Marek
David Herrmann Jan. 16, 2016, 7:46 p.m. UTC | #3
Hi

On Fri, Jan 15, 2016 at 4:24 PM, Marek Olšák <maraeo@gmail.com> wrote:
> On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> It warns for all "{}" initializers. Well, I want us to use {}.
>>> ---
>>>  configure.ac         | 3 ++-
>>>  intel/intel_decode.c | 2 --
>> The whole of libdrm, minus the intel_decode can get away without using
>> such constructs. And yes that includes radeon and amdgpu.
>>
>> NACK on this one - please be consistent with existing code base.
>
> Consistent with what? {} is the same as memset on each structure
> member. The warning says that a structure member is initialized to
> zero because of {}, which is why {} is used in the first place. It's
> the same as using memset and getting a warning "memset initializes the
> memory to zero". How useful is that?

The only use of this warning is to prevent people from learning that
{} initializes any non-specified field to 0.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David
Ilia Mirkin Jan. 16, 2016, 7:49 p.m. UTC | #4
Reviewed-by: Ilia Mirkin <imirkin@alum.mit.edu>

On Tue, Jan 12, 2016 at 4:14 PM, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> It warns for all "{}" initializers. Well, I want us to use {}.
> ---
>  configure.ac         | 3 ++-
>  intel/intel_decode.c | 2 --
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index c8c4ace..057a846 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -174,7 +174,8 @@ MAYBE_WARN="-Wall -Wextra \
>  -Wstrict-aliasing=2 -Winit-self \
>  -Wdeclaration-after-statement -Wold-style-definition \
>  -Wno-unused-parameter \
> --Wno-attributes -Wno-long-long -Winline -Wshadow"
> +-Wno-attributes -Wno-long-long -Winline -Wshadow \
> +-Wno-missing-field-initializers"
>
>  # invalidate cached value if MAYBE_WARN has changed
>  if test "x$libdrm_cv_warn_maybe" != "x$MAYBE_WARN"; then
> diff --git a/intel/intel_decode.c b/intel/intel_decode.c
> index e7aef74..287c342 100644
> --- a/intel/intel_decode.c
> +++ b/intel/intel_decode.c
> @@ -38,8 +38,6 @@
>  #include "intel_chipset.h"
>  #include "intel_bufmgr.h"
>
> -/* The compiler throws ~90 warnings. Do not spam the build, until we fix them. */
> -#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
>
>  /* Struct for tracking drm_intel_decode state. */
>  struct drm_intel_decode {
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Emil Velikov Jan. 18, 2016, 2:45 p.m. UTC | #5
On 15 January 2016 at 17:24, Marek Olšák <maraeo@gmail.com> wrote:
> On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> It warns for all "{}" initializers. Well, I want us to use {}.
>>> ---
>>>  configure.ac         | 3 ++-
>>>  intel/intel_decode.c | 2 --
>> The whole of libdrm, minus the intel_decode can get away without using
>> such constructs. And yes that includes radeon and amdgpu.
>>
>> NACK on this one - please be consistent with existing code base.
>
> Consistent with what? {} is the same as memset on each structure
> member. The warning says that a structure member is initialized to
> zero because of {}, which is why {} is used in the first place. It's
> the same as using memset and getting a warning "memset initializes the
> memory to zero". How useful is that?
>
There was a IRC discussion along the lines of "just use memset", but
for the sake of me I cannot find it.

> libdrm does have a lot of optional warnings enabled. Mesa does not,
> and Mesa does not even have this one. This means libdrm is
> inconsistent with Mesa and, BTW, it's also inconsistent with the kernel.
>
> It looks like somebody enabled optional warnings for libdrm in the
> past. All I'm doing is aligning the behavior with Mesa/kernel, which
> is what we would like to have and so would Intel apparently.
>
> Do you still think we are inconsistent?
>
If you look throughout libdrm you'll see - c99, {} (the one that's
causing you problems ?) and {0} initializers. ... And zero warnings
from Wmissing-field-initializers ? Don't know what your patch does,
but if things flag that normally means "you're doing something new".

If if bothers you that much - drop the warning. Just the next time
please don't go for "I want", it feels a bit ...

Thanks,
Emil
Marek Olšák Jan. 18, 2016, 3:43 p.m. UTC | #6
On Mon, Jan 18, 2016 at 3:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 15 January 2016 at 17:24, Marek Olšák <maraeo@gmail.com> wrote:
>> On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>
>>>> It warns for all "{}" initializers. Well, I want us to use {}.
>>>> ---
>>>>  configure.ac         | 3 ++-
>>>>  intel/intel_decode.c | 2 --
>>> The whole of libdrm, minus the intel_decode can get away without using
>>> such constructs. And yes that includes radeon and amdgpu.
>>>
>>> NACK on this one - please be consistent with existing code base.
>>
>> Consistent with what? {} is the same as memset on each structure
>> member. The warning says that a structure member is initialized to
>> zero because of {}, which is why {} is used in the first place. It's
>> the same as using memset and getting a warning "memset initializes the
>> memory to zero". How useful is that?
>>
> There was a IRC discussion along the lines of "just use memset", but
> for the sake of me I cannot find it.
>
>> libdrm does have a lot of optional warnings enabled. Mesa does not,
>> and Mesa does not even have this one. This means libdrm is
>> inconsistent with Mesa and, BTW, it's also inconsistent with the kernel.
>>
>> It looks like somebody enabled optional warnings for libdrm in the
>> past. All I'm doing is aligning the behavior with Mesa/kernel, which
>> is what we would like to have and so would Intel apparently.
>>
>> Do you still think we are inconsistent?
>>
> If you look throughout libdrm you'll see - c99, {} (the one that's
> causing you problems ?) and {0} initializers. ... And zero warnings
> from Wmissing-field-initializers ? Don't know what your patch does,
> but if things flag that normally means "you're doing something new".
>
> If if bothers you that much - drop the warning. Just the next time
> please don't go for "I want", it feels a bit ...

over the top? Sorry about that.

The thing is libdrm enables too many warnings. It's annoying and they
caused quite a lot of emotional discussion inside AMD. This is in configure.ac:

MAYBE_WARN="-Wall -Wextra \
-Wsign-compare -Werror-implicit-function-declaration \
-Wpointer-arith -Wwrite-strings -Wstrict-prototypes \
-Wmissing-prototypes -Wmissing-declarations -Wnested-externs \
-Wpacked -Wswitch-enum -Wmissing-format-attribute \
-Wstrict-aliasing=2 -Winit-self \
-Wdeclaration-after-statement -Wold-style-definition \
-Wno-unused-parameter \
-Wno-attributes -Wno-long-long -Winline -Wshadow

Marek
Jani Nikula Jan. 18, 2016, 3:51 p.m. UTC | #7
On Mon, 18 Jan 2016, Marek Olšák <maraeo@gmail.com> wrote:
> On Mon, Jan 18, 2016 at 3:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 15 January 2016 at 17:24, Marek Olšák <maraeo@gmail.com> wrote:
>>> On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>>
>>>>> It warns for all "{}" initializers. Well, I want us to use {}.
>>>>> ---
>>>>>  configure.ac         | 3 ++-
>>>>>  intel/intel_decode.c | 2 --
>>>> The whole of libdrm, minus the intel_decode can get away without using
>>>> such constructs. And yes that includes radeon and amdgpu.
>>>>
>>>> NACK on this one - please be consistent with existing code base.
>>>
>>> Consistent with what? {} is the same as memset on each structure
>>> member. The warning says that a structure member is initialized to
>>> zero because of {}, which is why {} is used in the first place. It's
>>> the same as using memset and getting a warning "memset initializes the
>>> memory to zero". How useful is that?
>>>
>> There was a IRC discussion along the lines of "just use memset", but
>> for the sake of me I cannot find it.
>>
>>> libdrm does have a lot of optional warnings enabled. Mesa does not,
>>> and Mesa does not even have this one. This means libdrm is
>>> inconsistent with Mesa and, BTW, it's also inconsistent with the kernel.
>>>
>>> It looks like somebody enabled optional warnings for libdrm in the
>>> past. All I'm doing is aligning the behavior with Mesa/kernel, which
>>> is what we would like to have and so would Intel apparently.
>>>
>>> Do you still think we are inconsistent?
>>>
>> If you look throughout libdrm you'll see - c99, {} (the one that's
>> causing you problems ?) and {0} initializers. ... And zero warnings
>> from Wmissing-field-initializers ? Don't know what your patch does,
>> but if things flag that normally means "you're doing something new".
>>
>> If if bothers you that much - drop the warning. Just the next time
>> please don't go for "I want", it feels a bit ...
>
> over the top? Sorry about that.
>
> The thing is libdrm enables too many warnings. It's annoying and they
> caused quite a lot of emotional discussion inside AMD. This is in configure.ac:

Please try upgrading your compiler.

BR,
Jani.

>
> MAYBE_WARN="-Wall -Wextra \
> -Wsign-compare -Werror-implicit-function-declaration \
> -Wpointer-arith -Wwrite-strings -Wstrict-prototypes \
> -Wmissing-prototypes -Wmissing-declarations -Wnested-externs \
> -Wpacked -Wswitch-enum -Wmissing-format-attribute \
> -Wstrict-aliasing=2 -Winit-self \
> -Wdeclaration-after-statement -Wold-style-definition \
> -Wno-unused-parameter \
> -Wno-attributes -Wno-long-long -Winline -Wshadow
>
> Marek
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Emil Velikov Jan. 18, 2016, 4:05 p.m. UTC | #8
On 18 January 2016 at 17:43, Marek Olšák <maraeo@gmail.com> wrote:
> On Mon, Jan 18, 2016 at 3:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 15 January 2016 at 17:24, Marek Olšák <maraeo@gmail.com> wrote:
>>> On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>>
>>>>> It warns for all "{}" initializers. Well, I want us to use {}.
>>>>> ---
>>>>>  configure.ac         | 3 ++-
>>>>>  intel/intel_decode.c | 2 --
>>>> The whole of libdrm, minus the intel_decode can get away without using
>>>> such constructs. And yes that includes radeon and amdgpu.
>>>>
>>>> NACK on this one - please be consistent with existing code base.
>>>
>>> Consistent with what? {} is the same as memset on each structure
>>> member. The warning says that a structure member is initialized to
>>> zero because of {}, which is why {} is used in the first place. It's
>>> the same as using memset and getting a warning "memset initializes the
>>> memory to zero". How useful is that?
>>>
>> There was a IRC discussion along the lines of "just use memset", but
>> for the sake of me I cannot find it.
>>
>>> libdrm does have a lot of optional warnings enabled. Mesa does not,
>>> and Mesa does not even have this one. This means libdrm is
>>> inconsistent with Mesa and, BTW, it's also inconsistent with the kernel.
>>>
>>> It looks like somebody enabled optional warnings for libdrm in the
>>> past. All I'm doing is aligning the behavior with Mesa/kernel, which
>>> is what we would like to have and so would Intel apparently.
>>>
>>> Do you still think we are inconsistent?
>>>
>> If you look throughout libdrm you'll see - c99, {} (the one that's
>> causing you problems ?) and {0} initializers. ... And zero warnings
>> from Wmissing-field-initializers ? Don't know what your patch does,
>> but if things flag that normally means "you're doing something new".
>>
>> If if bothers you that much - drop the warning. Just the next time
>> please don't go for "I want", it feels a bit ...
>
> over the top? Sorry about that.
>
Precisely. Apology accepted :-)

> The thing is libdrm enables too many warnings. It's annoying and they
> caused quite a lot of emotional discussion inside AMD. This is in configure.ac:
>
> MAYBE_WARN="-Wall -Wextra \
> -Wsign-compare -Werror-implicit-function-declaration \
> -Wpointer-arith -Wwrite-strings -Wstrict-prototypes \
> -Wmissing-prototypes -Wmissing-declarations -Wnested-externs \
> -Wpacked -Wswitch-enum -Wmissing-format-attribute \
> -Wstrict-aliasing=2 -Winit-self \
> -Wdeclaration-after-statement -Wold-style-definition \
> -Wno-unused-parameter \
> -Wno-attributes -Wno-long-long -Winline -Wshadow
>
A few of those are already implicit with either Wall or Wextra. Both
of which, imho, are a must have for any serious project. If you want
we can nuke the -Wno-foo ones :-P

But seriously - it makes me think that people are rushed to write the
code and get it out. Or perhaps a too strong "no warnings" policy ?
After all warnings are to hint that things can be improved/might be
wrong. If it looks trivial, just ignore it :-)

Cheers,
Emil
Marek Olšák Jan. 18, 2016, 10:53 p.m. UTC | #9
On Mon, Jan 18, 2016 at 5:05 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 18 January 2016 at 17:43, Marek Olšák <maraeo@gmail.com> wrote:
>> On Mon, Jan 18, 2016 at 3:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 15 January 2016 at 17:24, Marek Olšák <maraeo@gmail.com> wrote:
>>>> On Fri, Jan 15, 2016 at 12:12 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>> On 12 January 2016 at 23:14, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>>>
>>>>>> It warns for all "{}" initializers. Well, I want us to use {}.
>>>>>> ---
>>>>>>  configure.ac         | 3 ++-
>>>>>>  intel/intel_decode.c | 2 --
>>>>> The whole of libdrm, minus the intel_decode can get away without using
>>>>> such constructs. And yes that includes radeon and amdgpu.
>>>>>
>>>>> NACK on this one - please be consistent with existing code base.
>>>>
>>>> Consistent with what? {} is the same as memset on each structure
>>>> member. The warning says that a structure member is initialized to
>>>> zero because of {}, which is why {} is used in the first place. It's
>>>> the same as using memset and getting a warning "memset initializes the
>>>> memory to zero". How useful is that?
>>>>
>>> There was a IRC discussion along the lines of "just use memset", but
>>> for the sake of me I cannot find it.
>>>
>>>> libdrm does have a lot of optional warnings enabled. Mesa does not,
>>>> and Mesa does not even have this one. This means libdrm is
>>>> inconsistent with Mesa and, BTW, it's also inconsistent with the kernel.
>>>>
>>>> It looks like somebody enabled optional warnings for libdrm in the
>>>> past. All I'm doing is aligning the behavior with Mesa/kernel, which
>>>> is what we would like to have and so would Intel apparently.
>>>>
>>>> Do you still think we are inconsistent?
>>>>
>>> If you look throughout libdrm you'll see - c99, {} (the one that's
>>> causing you problems ?) and {0} initializers. ... And zero warnings
>>> from Wmissing-field-initializers ? Don't know what your patch does,
>>> but if things flag that normally means "you're doing something new".
>>>
>>> If if bothers you that much - drop the warning. Just the next time
>>> please don't go for "I want", it feels a bit ...
>>
>> over the top? Sorry about that.
>>
> Precisely. Apology accepted :-)
>
>> The thing is libdrm enables too many warnings. It's annoying and they
>> caused quite a lot of emotional discussion inside AMD. This is in configure.ac:
>>
>> MAYBE_WARN="-Wall -Wextra \
>> -Wsign-compare -Werror-implicit-function-declaration \
>> -Wpointer-arith -Wwrite-strings -Wstrict-prototypes \
>> -Wmissing-prototypes -Wmissing-declarations -Wnested-externs \
>> -Wpacked -Wswitch-enum -Wmissing-format-attribute \
>> -Wstrict-aliasing=2 -Winit-self \
>> -Wdeclaration-after-statement -Wold-style-definition \
>> -Wno-unused-parameter \
>> -Wno-attributes -Wno-long-long -Winline -Wshadow
>>
> A few of those are already implicit with either Wall or Wextra. Both
> of which, imho, are a must have for any serious project. If you want
> we can nuke the -Wno-foo ones :-P
>
> But seriously - it makes me think that people are rushed to write the
> code and get it out. Or perhaps a too strong "no warnings" policy ?
> After all warnings are to hint that things can be improved/might be
> wrong. If it looks trivial, just ignore it :-)

Try explaining that to people who have a compulsion to fix them or
argue about them. :) Ignore? REALLY? IGNORE???

Marek
Michel Dänzer Jan. 19, 2016, 3:42 a.m. UTC | #10
On 19.01.2016 01:05, Emil Velikov wrote:
> 
> A few of those are already implicit with either Wall or Wextra. Both
> of which, imho, are a must have for any serious project.

I think -Wextra is generally too noisy for that, but I guess we're now
deeply in arguing about taste territory.


> But seriously - it makes me think that people are rushed to write the
> code and get it out. Or perhaps a too strong "no warnings" policy ?
> After all warnings are to hint that things can be improved/might be
> wrong. If it looks trivial, just ignore it :-)

One problem with that is that leaving trivial/irrelevant/incorrect
warnings makes it easier to miss important warnings. That being said, I
fully agree that one should resist the urge to just get rid of warnings
in whatever way. (I tend to cringe whenever a commit log says something
along the lines of "fix warning" — a change either fixes a problem which
was pointed out by the warning, or it just silences the warning.)
Emil Velikov Jan. 21, 2016, 10:51 a.m. UTC | #11
On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
> Try explaining that to people who have a compulsion to fix them or
> argue about them. :) Ignore? REALLY? IGNORE???
>
Now that we have a few people off your back can you please point out
where this triggers warnings ?
I've tried multiple times to get some but I'm falling short. Iirc this
warning helped my catch an issue when cunit broke their ABI.

Also I would kindly invite everyone else interested to speak publicly
about their conserns - it is an FOSS project after all :-)

Thanks
Emil
Marek Olšák Jan. 21, 2016, 12:08 p.m. UTC | #12
On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>> Try explaining that to people who have a compulsion to fix them or
>> argue about them. :) Ignore? REALLY? IGNORE???
>>
> Now that we have a few people off your back can you please point out
> where this triggers warnings ?

This particular warning is trigged by {} or any { ... } which doesn't
initialize all members.

Marek
Emil Velikov Jan. 21, 2016, 1:09 p.m. UTC | #13
On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>> Try explaining that to people who have a compulsion to fix them or
>>> argue about them. :) Ignore? REALLY? IGNORE???
>>>
>> Now that we have a few people off your back can you please point out
>> where this triggers warnings ?
>
> This particular warning is trigged by {}
As mentioned previously neither {} nor {0} trigger any warning here.
Jani hinted that you might be using an old (buggy?) compiler which
generates them.
Which version of GCC are you using ? Do you mind showing the first few
warnings ?

> or any { ... } which doesn't
> initialize all members.
>
Do we have any outside of intel_decode.c ? I'm failing to spot any.

-Emil
Marek Olšák Jan. 21, 2016, 4:58 p.m. UTC | #14
On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>>> Try explaining that to people who have a compulsion to fix them or
>>>> argue about them. :) Ignore? REALLY? IGNORE???
>>>>
>>> Now that we have a few people off your back can you please point out
>>> where this triggers warnings ?
>>
>> This particular warning is trigged by {}
> As mentioned previously neither {} nor {0} trigger any warning here.
> Jani hinted that you might be using an old (buggy?) compiler which
> generates them.
> Which version of GCC are you using ? Do you mind showing the first few
> warnings ?
>
>> or any { ... } which doesn't
>> initialize all members.
>>
> Do we have any outside of intel_decode.c ? I'm failing to spot any.

amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
There are more in libdrm. I have gcc 4.9.2. If I revert this patch, I
get this nice log:


xf86drmMode.c: In function ‘drmHandleEvent’:
xf86drmMode.c:891:15: warning: dereferencing type-punned pointer will
break strict-aliasing rules [-Wstrict-aliasing]
   e = (struct drm_event *) &buffer[i];
               ^
abi16.c: In function ‘abi16_chan_nvc0’:
abi16.c:66:9: warning: missing initializer for field
‘fb_ctxdma_handle’ of ‘struct drm_nouveau_channel_alloc’
[-Wmissing-field-initializers]
  struct drm_nouveau_channel_alloc req = {};
         ^
In file included from private.h:8:0,
                 from abi16.c:34:
../include/drm/nouveau_drm.h:31:11: note: ‘fb_ctxdma_handle’ declared here
  uint32_t     fb_ctxdma_handle;
           ^
abi16.c: In function ‘abi16_chan_nve0’:
abi16.c:87:9: warning: missing initializer for field
‘fb_ctxdma_handle’ of ‘struct drm_nouveau_channel_alloc’
[-Wmissing-field-initializers]
  struct drm_nouveau_channel_alloc req = {};
         ^
In file included from private.h:8:0,
                 from abi16.c:34:
../include/drm/nouveau_drm.h:31:11: note: ‘fb_ctxdma_handle’ declared here
  uint32_t     fb_ctxdma_handle;
           ^
abi16.c: In function ‘abi16_bo_init’:
abi16.c:316:9: warning: missing initializer for field ‘info’ of
‘struct drm_nouveau_gem_new’ [-Wmissing-field-initializers]
  struct drm_nouveau_gem_new req = {};
         ^
In file included from private.h:8:0,
                 from abi16.c:34:
../include/drm/nouveau_drm.h:118:30: note: ‘info’ declared here
  struct drm_nouveau_gem_info info;
                              ^
nouveau.c: In function ‘nouveau_object_fini’:
nouveau.c:222:2: warning: missing initializer for field ‘pad02’ of
‘struct nvif_ioctl_v0’ [-Wmissing-field-initializers]
  };
  ^
In file included from nouveau.c:50:0:
nvif/ioctl.h:22:7: note: ‘pad02’ declared here
  __u8  pad02[4];
       ^
nouveau.c: In function ‘nouveau_device_new’:
nouveau.c:371:9: warning: missing initializer for field ‘version’ of
‘struct nv_device_info_v0’ [-Wmissing-field-initializers]
  struct nv_device_info_v0 info = {};
         ^
In file included from nouveau.c:49:0:
nvif/cl0080.h:14:7: note: ‘version’ declared here
  __u8  version;
       ^
pushbuf.c: In function ‘nouveau_pushbuf_new’:
pushbuf.c:544:9: warning: missing initializer for field ‘channel’ of
‘struct drm_nouveau_gem_pushbuf’ [-Wmissing-field-initializers]
  struct drm_nouveau_gem_pushbuf req = {};
         ^
In file included from pushbuf.c:40:0:
../include/drm/nouveau_drm.h:162:11: note: ‘channel’ declared here
  uint32_t channel;
           ^
radeon_bo_gem.c: In function ‘bo_get_tiling’:
radeon_bo_gem.c:255:12: warning: missing initializer for field
‘handle’ of ‘struct drm_radeon_gem_set_tiling’
[-Wmissing-field-initializers]
     struct drm_radeon_gem_set_tiling args = {};
            ^
In file included from radeon_bo_gem.c:44:0:
../include/drm/radeon_drm.h:829:11: note: ‘handle’ declared here
  uint32_t handle;
           ^
radeon_cs_gem.c: In function ‘radeon_get_device_id’:
radeon_cs_gem.c:531:12: warning: missing initializer for field
‘request’ of ‘struct drm_radeon_info’ [-Wmissing-field-initializers]
     struct drm_radeon_info info = {};
            ^
In file included from radeon_cs.h:38:0,
                 from radeon_cs_gem.c:41:
../include/drm/radeon_drm.h:1014:11: note: ‘request’ declared here
  uint32_t  request;
           ^
radeon_surface.c: In function ‘radeon_get_value’:
radeon_surface.c:121:12: warning: missing initializer for field
‘request’ of ‘struct drm_radeon_info’ [-Wmissing-field-initializers]
     struct drm_radeon_info info = {};
            ^
In file included from radeon_surface.c:42:0:
../include/drm/radeon_drm.h:1014:11: note: ‘request’ declared here
  uint32_t  request;
           ^
amdgpu_bo.c: In function ‘amdgpu_close_kms_handle’:
amdgpu_bo.c:50:9: warning: missing initializer for field ‘handle’ of
‘struct drm_gem_close’ [-Wmissing-field-initializers]
  struct drm_gem_close args = {};
         ^
In file included from ../xf86drm.h:40:0,
                 from amdgpu_bo.c:41:
../include/drm/drm.h:590:8: note: ‘handle’ declared here
  __u32 handle;
        ^
amdgpu_bo.c: In function ‘amdgpu_bo_set_metadata’:
amdgpu_bo.c:127:9: warning: missing initializer for field ‘handle’ of
‘struct drm_amdgpu_gem_metadata’ [-Wmissing-field-initializers]
  struct drm_amdgpu_gem_metadata args = {};
         ^
In file included from amdgpu_bo.c:42:0:
../include/drm/amdgpu_drm.h:231:11: note: ‘handle’ declared here
  uint32_t handle;
           ^
amdgpu_bo.c: In function ‘amdgpu_bo_query_info’:
amdgpu_bo.c:150:9: warning: missing initializer for field ‘handle’ of
‘struct drm_amdgpu_gem_metadata’ [-Wmissing-field-initializers]
  struct drm_amdgpu_gem_metadata metadata = {};
         ^
In file included from amdgpu_bo.c:42:0:
../include/drm/amdgpu_drm.h:231:11: note: ‘handle’ declared here
  uint32_t handle;
           ^
amdgpu_bo.c:151:9: warning: missing initializer for field ‘bo_size’ of
‘struct drm_amdgpu_gem_create_in’ [-Wmissing-field-initializers]
  struct drm_amdgpu_gem_create_in bo_info = {};
         ^
In file included from amdgpu_bo.c:42:0:
../include/drm/amdgpu_drm.h:81:11: note: ‘bo_size’ declared here
  uint64_t bo_size;
           ^
amdgpu_bo.c:152:9: warning: missing initializer for field ‘handle’ of
‘struct drm_amdgpu_gem_op’ [-Wmissing-field-initializers]
  struct drm_amdgpu_gem_op gem_op = {};
         ^
In file included from amdgpu_bo.c:42:0:
../include/drm/amdgpu_drm.h:334:11: note: ‘handle’ declared here
  uint32_t handle;
           ^
amdgpu_bo.c: In function ‘amdgpu_bo_export_flink’:
amdgpu_bo.c:240:10: warning: missing initializer for field ‘handle’ of
‘struct drm_gem_close’ [-Wmissing-field-initializers]
   struct drm_gem_close args = {};
          ^
In file included from ../xf86drm.h:40:0,
                 from amdgpu_bo.c:41:
../include/drm/drm.h:590:8: note: ‘handle’ declared here
  __u32 handle;
        ^
amdgpu_bo.c: In function ‘amdgpu_bo_import’:
amdgpu_bo.c:287:9: warning: missing initializer for field ‘name’ of
‘struct drm_gem_open’ [-Wmissing-field-initializers]
  struct drm_gem_open open_arg = {};
         ^
In file included from ../xf86drm.h:40:0,
                 from amdgpu_bo.c:41:
../include/drm/drm.h:606:8: note: ‘name’ declared here
  __u32 name;
        ^
amdgpu_gpu_info.c: In function ‘amdgpu_query_heap_info’:
amdgpu_gpu_info.c:240:9: warning: missing initializer for field
‘vram_size’ of ‘struct drm_amdgpu_info_vram_gtt’
[-Wmissing-field-initializers]
  struct drm_amdgpu_info_vram_gtt vram_gtt_info = {};
         ^
In file included from amdgpu_gpu_info.c:33:0:
../include/drm/amdgpu_drm.h:586:11: note: ‘vram_size’ declared here
  uint64_t vram_size;
           ^
amdgpu_gpu_info.c: In function ‘amdgpu_query_gds_info’:
amdgpu_gpu_info.c:290:9: warning: missing initializer for field
‘gds_gfx_partition_size’ of ‘struct drm_amdgpu_info_gds’
[-Wmissing-field-initializers]
  struct drm_amdgpu_info_gds gds_config = {};
         ^
In file included from amdgpu_gpu_info.c:33:0:
../include/drm/amdgpu_drm.h:569:11: note: ‘gds_gfx_partition_size’ declared here
  uint32_t gds_gfx_partition_size;
           ^
amdgpu_device.c: In function ‘amdgpu_get_auth’:
amdgpu_device.c:118:2: warning: missing initializer for field ‘idx’ of
‘drm_client_t’ [-Wmissing-field-initializers]
  drm_client_t client = {};
  ^
In file included from ../xf86drm.h:40:0,
                 from amdgpu_device.c:42:
../include/drm/drm.h:227:6: note: ‘idx’ declared here
  int idx;  /**< Which client desired? */
      ^
kms-universal-planes.c: In function ‘main’:
kms-universal-planes.c:212:22: warning: declaration of ‘plane’ shadows
a previous local [-Wshadow]
    struct kms_plane *plane = device->planes[i];
                      ^
kms-universal-planes.c:137:20: warning: shadowed declaration is here [-Wshadow]
  struct kms_plane *plane;
                    ^
modetest.c: In function ‘get_resources’:
modetest.c:559:3: warning: ignoring return value of ‘asprintf’,
declared with attribute warn_unused_result [-Wunused-result]
   asprintf(&connector->name, "%s-%u",
   ^
basic_tests.c: In function ‘amdgpu_userptr_test’:
basic_tests.c:1028:2: warning: ignoring return value of
‘posix_memalign’, declared with attribute warn_unused_result
[-Wunused-result]
  posix_memalign(&ptr, sysconf(_SC_PAGE_SIZE), BUFFER_SIZE);
  ^

Marek
Emil Velikov Jan. 22, 2016, 5:13 p.m. UTC | #15
On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> Try explaining that to people who have a compulsion to fix them or
>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>>>>>
>>>> Now that we have a few people off your back can you please point out
>>>> where this triggers warnings ?
>>>
>>> This particular warning is trigged by {}
>> As mentioned previously neither {} nor {0} trigger any warning here.
>> Jani hinted that you might be using an old (buggy?) compiler which
>> generates them.
>> Which version of GCC are you using ? Do you mind showing the first few
>> warnings ?
>>
>>> or any { ... } which doesn't
>>> initialize all members.
>>>
>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>
> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
send a patch to transition to either one of these two ?

> There are more in libdrm. I have gcc 4.9.2. If I revert this patch, I
> get this nice log:
>
Interesting... I could swear I've tried the patch with gcc 4.9.x
(currenly using 5.x). Hmm at the same time gcc does not seem to list
any -Wunused-result warnings here.

Thanks
Emil
Marek Olšák Jan. 22, 2016, 5:18 p.m. UTC | #16
On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> Try explaining that to people who have a compulsion to fix them or
>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>>>>>>
>>>>> Now that we have a few people off your back can you please point out
>>>>> where this triggers warnings ?
>>>>
>>>> This particular warning is trigged by {}
>>> As mentioned previously neither {} nor {0} trigger any warning here.
>>> Jani hinted that you might be using an old (buggy?) compiler which
>>> generates them.
>>> Which version of GCC are you using ? Do you mind showing the first few
>>> warnings ?
>>>
>>>> or any { ... } which doesn't
>>>> initialize all members.
>>>>
>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>>
>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
> send a patch to transition to either one of these two ?

That's up to you, but please note that I don't plan to stop using "= {}",
because it's the most convenient way to clear memory in a lot of
cases and takes only 4 bytes of text.

Marek
Ilia Mirkin Jan. 22, 2016, 5:29 p.m. UTC | #17
On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>> Try explaining that to people who have a compulsion to fix them or
>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>>>>>>>
>>>>>> Now that we have a few people off your back can you please point out
>>>>>> where this triggers warnings ?
>>>>>
>>>>> This particular warning is trigged by {}
>>>> As mentioned previously neither {} nor {0} trigger any warning here.
>>>> Jani hinted that you might be using an old (buggy?) compiler which
>>>> generates them.
>>>> Which version of GCC are you using ? Do you mind showing the first few
>>>> warnings ?
>>>>
>>>>> or any { ... } which doesn't
>>>>> initialize all members.
>>>>>
>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>>>
>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
>> send a patch to transition to either one of these two ?
>
> That's up to you, but please note that I don't plan to stop using "= {}",
> because it's the most convenient way to clear memory in a lot of
> cases and takes only 4 bytes of text.

I like {} too and think we should encourage that. I'd rather
transition the { 0 } stuff over to {}.

  -ilia
Emil Velikov Jan. 22, 2016, 5:40 p.m. UTC | #18
On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
>>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>>> Try explaining that to people who have a compulsion to fix them or
>>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>>>>>>>>
>>>>>>> Now that we have a few people off your back can you please point out
>>>>>>> where this triggers warnings ?
>>>>>>
>>>>>> This particular warning is trigged by {}
>>>>> As mentioned previously neither {} nor {0} trigger any warning here.
>>>>> Jani hinted that you might be using an old (buggy?) compiler which
>>>>> generates them.
>>>>> Which version of GCC are you using ? Do you mind showing the first few
>>>>> warnings ?
>>>>>
>>>>>> or any { ... } which doesn't
>>>>>> initialize all members.
>>>>>>
>>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>>>>
>>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
>>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
>>> send a patch to transition to either one of these two ?
>>
>> That's up to you, but please note that I don't plan to stop using "= {}",
>> because it's the most convenient way to clear memory in a lot of
>> cases and takes only 4 bytes of text.
>
> I like {} too and think we should encourage that. I'd rather
> transition the { 0 } stuff over to {}.
>
So people feel against seeing/writing single extra character 0,
despite that the warning has helped catch actual bug ?
And now are willing to transitions 40+ cases as opposed to ~15... that
feels strange to say the least.

-Emil
Ville Syrjala Jan. 22, 2016, 5:47 p.m. UTC | #19
On Fri, Jan 22, 2016 at 05:40:54PM +0000, Emil Velikov wrote:
> On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> > On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
> >> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
> >>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
> >>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
> >>>>>>>> Try explaining that to people who have a compulsion to fix them or
> >>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
> >>>>>>>>
> >>>>>>> Now that we have a few people off your back can you please point out
> >>>>>>> where this triggers warnings ?
> >>>>>>
> >>>>>> This particular warning is trigged by {}
> >>>>> As mentioned previously neither {} nor {0} trigger any warning here.
> >>>>> Jani hinted that you might be using an old (buggy?) compiler which
> >>>>> generates them.
> >>>>> Which version of GCC are you using ? Do you mind showing the first few
> >>>>> warnings ?
> >>>>>
> >>>>>> or any { ... } which doesn't
> >>>>>> initialize all members.
> >>>>>>
> >>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
> >>>>
> >>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
> >>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
> >>> send a patch to transition to either one of these two ?
> >>
> >> That's up to you, but please note that I don't plan to stop using "= {}",
> >> because it's the most convenient way to clear memory in a lot of
> >> cases and takes only 4 bytes of text.
> >
> > I like {} too and think we should encourage that. I'd rather
> > transition the { 0 } stuff over to {}.
> >
> So people feel against seeing/writing single extra character 0,
> despite that the warning has helped catch actual bug ?
> And now are willing to transitions 40+ cases as opposed to ~15... that
> feels strange to say the least.

Does the '= { 0 }' thing even work if the first member happens to be
something other than an integer?
Ilia Mirkin Jan. 22, 2016, 5:48 p.m. UTC | #20
On Fri, Jan 22, 2016 at 12:47 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Jan 22, 2016 at 05:40:54PM +0000, Emil Velikov wrote:
>> On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> > On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> >> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
>> >>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>> >>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>> >>>>>>>> Try explaining that to people who have a compulsion to fix them or
>> >>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>> >>>>>>>>
>> >>>>>>> Now that we have a few people off your back can you please point out
>> >>>>>>> where this triggers warnings ?
>> >>>>>>
>> >>>>>> This particular warning is trigged by {}
>> >>>>> As mentioned previously neither {} nor {0} trigger any warning here.
>> >>>>> Jani hinted that you might be using an old (buggy?) compiler which
>> >>>>> generates them.
>> >>>>> Which version of GCC are you using ? Do you mind showing the first few
>> >>>>> warnings ?
>> >>>>>
>> >>>>>> or any { ... } which doesn't
>> >>>>>> initialize all members.
>> >>>>>>
>> >>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>> >>>>
>> >>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
>> >>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
>> >>> send a patch to transition to either one of these two ?
>> >>
>> >> That's up to you, but please note that I don't plan to stop using "= {}",
>> >> because it's the most convenient way to clear memory in a lot of
>> >> cases and takes only 4 bytes of text.
>> >
>> > I like {} too and think we should encourage that. I'd rather
>> > transition the { 0 } stuff over to {}.
>> >
>> So people feel against seeing/writing single extra character 0,
>> despite that the warning has helped catch actual bug ?
>> And now are willing to transitions 40+ cases as opposed to ~15... that
>> feels strange to say the least.
>
> Does the '= { 0 }' thing even work if the first member happens to be
> something other than an integer?

No. That's why I like {}. Otherwise you end up doing {{{{{{{{{{0}}}}}}}}}.

  -ilia
Emil Velikov Jan. 22, 2016, 5:50 p.m. UTC | #21
On 22 January 2016 at 17:47, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Jan 22, 2016 at 05:40:54PM +0000, Emil Velikov wrote:
>> On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> > On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> >> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
>> >>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>> >>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> >>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>> >>>>>>>> Try explaining that to people who have a compulsion to fix them or
>> >>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>> >>>>>>>>
>> >>>>>>> Now that we have a few people off your back can you please point out
>> >>>>>>> where this triggers warnings ?
>> >>>>>>
>> >>>>>> This particular warning is trigged by {}
>> >>>>> As mentioned previously neither {} nor {0} trigger any warning here.
>> >>>>> Jani hinted that you might be using an old (buggy?) compiler which
>> >>>>> generates them.
>> >>>>> Which version of GCC are you using ? Do you mind showing the first few
>> >>>>> warnings ?
>> >>>>>
>> >>>>>> or any { ... } which doesn't
>> >>>>>> initialize all members.
>> >>>>>>
>> >>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>> >>>>
>> >>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
>> >>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
>> >>> send a patch to transition to either one of these two ?
>> >>
>> >> That's up to you, but please note that I don't plan to stop using "= {}",
>> >> because it's the most convenient way to clear memory in a lot of
>> >> cases and takes only 4 bytes of text.
>> >
>> > I like {} too and think we should encourage that. I'd rather
>> > transition the { 0 } stuff over to {}.
>> >
>> So people feel against seeing/writing single extra character 0,
>> despite that the warning has helped catch actual bug ?
>> And now are willing to transitions 40+ cases as opposed to ~15... that
>> feels strange to say the least.
>
> Does the '= { 0 }' thing even work if the first member happens to be
> something other than an integer?
>
It does here with GCC 5.2.0 :-) Cannot comment about other compilers.

-Emil
Emil Velikov Jan. 22, 2016, 5:59 p.m. UTC | #22
On 22 January 2016 at 17:50, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 22 January 2016 at 17:47, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Jan 22, 2016 at 05:40:54PM +0000, Emil Velikov wrote:
>>> On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>> > On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>> >> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> >>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
>>> >>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> >>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
>>> >>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> >>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
>>> >>>>>>>> Try explaining that to people who have a compulsion to fix them or
>>> >>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
>>> >>>>>>>>
>>> >>>>>>> Now that we have a few people off your back can you please point out
>>> >>>>>>> where this triggers warnings ?
>>> >>>>>>
>>> >>>>>> This particular warning is trigged by {}
>>> >>>>> As mentioned previously neither {} nor {0} trigger any warning here.
>>> >>>>> Jani hinted that you might be using an old (buggy?) compiler which
>>> >>>>> generates them.
>>> >>>>> Which version of GCC are you using ? Do you mind showing the first few
>>> >>>>> warnings ?
>>> >>>>>
>>> >>>>>> or any { ... } which doesn't
>>> >>>>>> initialize all members.
>>> >>>>>>
>>> >>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
>>> >>>>
>>> >>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
>>> >>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
>>> >>> send a patch to transition to either one of these two ?
>>> >>
>>> >> That's up to you, but please note that I don't plan to stop using "= {}",
>>> >> because it's the most convenient way to clear memory in a lot of
>>> >> cases and takes only 4 bytes of text.
>>> >
>>> > I like {} too and think we should encourage that. I'd rather
>>> > transition the { 0 } stuff over to {}.
>>> >
>>> So people feel against seeing/writing single extra character 0,
>>> despite that the warning has helped catch actual bug ?
>>> And now are willing to transitions 40+ cases as opposed to ~15... that
>>> feels strange to say the least.
>>
>> Does the '= { 0 }' thing even work if the first member happens to be
>> something other than an integer?
>>
> It does here with GCC 5.2.0 :-) Cannot comment about other compilers.
>
Also let's not forget about

a.c:17:20: warning: ISO C forbids empty initializer braces [-Wpedantic]
     struct foo f = {};
                    ^

-Emil
Ville Syrjala Jan. 22, 2016, 6:49 p.m. UTC | #23
On Fri, Jan 22, 2016 at 05:59:30PM +0000, Emil Velikov wrote:
> On 22 January 2016 at 17:50, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > On 22 January 2016 at 17:47, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Fri, Jan 22, 2016 at 05:40:54PM +0000, Emil Velikov wrote:
> >>> On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> >>> > On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com> wrote:
> >>> >> On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>> >>> On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com> wrote:
> >>> >>>> On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>> >>>>> On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.com> wrote:
> >>> >>>>>> On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>> >>>>>>> On 18 January 2016 at 22:53, Marek Olšák <maraeo@gmail.com> wrote:
> >>> >>>>>>>> Try explaining that to people who have a compulsion to fix them or
> >>> >>>>>>>> argue about them. :) Ignore? REALLY? IGNORE???
> >>> >>>>>>>>
> >>> >>>>>>> Now that we have a few people off your back can you please point out
> >>> >>>>>>> where this triggers warnings ?
> >>> >>>>>>
> >>> >>>>>> This particular warning is trigged by {}
> >>> >>>>> As mentioned previously neither {} nor {0} trigger any warning here.
> >>> >>>>> Jani hinted that you might be using an old (buggy?) compiler which
> >>> >>>>> generates them.
> >>> >>>>> Which version of GCC are you using ? Do you mind showing the first few
> >>> >>>>> warnings ?
> >>> >>>>>
> >>> >>>>>> or any { ... } which doesn't
> >>> >>>>>> initialize all members.
> >>> >>>>>>
> >>> >>>>> Do we have any outside of intel_decode.c ? I'm failing to spot any.
> >>> >>>>
> >>> >>>> amdgpu_bo.c has 7 occurences of "= {}" and they all print the warning.
> >>> >>> With 200+ cases of memset and 40+ of "= *{ *0 *}". Any objections if I
> >>> >>> send a patch to transition to either one of these two ?
> >>> >>
> >>> >> That's up to you, but please note that I don't plan to stop using "= {}",
> >>> >> because it's the most convenient way to clear memory in a lot of
> >>> >> cases and takes only 4 bytes of text.
> >>> >
> >>> > I like {} too and think we should encourage that. I'd rather
> >>> > transition the { 0 } stuff over to {}.
> >>> >
> >>> So people feel against seeing/writing single extra character 0,
> >>> despite that the warning has helped catch actual bug ?
> >>> And now are willing to transitions 40+ cases as opposed to ~15... that
> >>> feels strange to say the least.
> >>
> >> Does the '= { 0 }' thing even work if the first member happens to be
> >> something other than an integer?
> >>
> > It does here with GCC 5.2.0 :-) Cannot comment about other compilers.
> >
> Also let's not forget about
> 
> a.c:17:20: warning: ISO C forbids empty initializer braces [-Wpedantic]
>      struct foo f = {};
>                     ^

I long ago decided that -pedantic is stupid, hence I don't use it.

My gcc (4.9.3 something) seems to allow the {0} but with a struct within
a struct it angers -Wmissing-braces, although my reading of the spec
suggests that it's pretty well defined how this sort of thing should
behave. I was expecting some kind of 'implicit pointer from integer'
warning when the thing it would initialize is a pointer, but didn't get
one. Not sure why.

And {} of course makes -Wmissing-field-initializers upset. I can't see
anything in the spec to relly forbid this form, except that the syntax
maybe doesn't allow for an empty initializer-list.

About the only "useful" thing I learned from the spec is that 0 is an
octal constant :) Makes some sense but it never occured to me before.

So I guess all I can say is that gcc is stupid, and it should just stfu
and let both '= {}' and '= {0}' through without whining about it.
Jan Vesely Jan. 22, 2016, 7:18 p.m. UTC | #24
On Fri, 2016-01-22 at 12:48 -0500, Ilia Mirkin wrote:
> On Fri, Jan 22, 2016 at 12:47 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jan 22, 2016 at 05:40:54PM +0000, Emil Velikov wrote:
> > > On 22 January 2016 at 17:29, Ilia Mirkin <imirkin@alum.mit.edu>
> > > wrote:
> > > > On Fri, Jan 22, 2016 at 12:18 PM, Marek Olšák <maraeo@gmail.com
> > > > > wrote:
> > > > > On Fri, Jan 22, 2016 at 6:13 PM, Emil Velikov <emil.l.velikov
> > > > > @gmail.com> wrote:
> > > > > > On 21 January 2016 at 16:58, Marek Olšák <maraeo@gmail.com>
> > > > > > wrote:
> > > > > > > On Thu, Jan 21, 2016 at 2:09 PM, Emil Velikov <emil.l.vel
> > > > > > > ikov@gmail.com> wrote:
> > > > > > > > On 21 January 2016 at 12:08, Marek Olšák <maraeo@gmail.
> > > > > > > > com> wrote:
> > > > > > > > > On Thu, Jan 21, 2016 at 11:51 AM, Emil Velikov <emil.
> > > > > > > > > l.velikov@gmail.com> wrote:
> > > > > > > > > > On 18 January 2016 at 22:53, Marek Olšák <maraeo@gm
> > > > > > > > > > ail.com> wrote:
> > > > > > > > > > > Try explaining that to people who have a
> > > > > > > > > > > compulsion to fix them or
> > > > > > > > > > > argue about them. :) Ignore? REALLY? IGNORE???
> > > > > > > > > > > 
> > > > > > > > > > Now that we have a few people off your back can you
> > > > > > > > > > please point out
> > > > > > > > > > where this triggers warnings ?
> > > > > > > > > 
> > > > > > > > > This particular warning is trigged by {}
> > > > > > > > As mentioned previously neither {} nor {0} trigger any
> > > > > > > > warning here.
> > > > > > > > Jani hinted that you might be using an old (buggy?)
> > > > > > > > compiler which
> > > > > > > > generates them.
> > > > > > > > Which version of GCC are you using ? Do you mind
> > > > > > > > showing the first few
> > > > > > > > warnings ?
> > > > > > > > 
> > > > > > > > > or any { ... } which doesn't
> > > > > > > > > initialize all members.
> > > > > > > > > 
> > > > > > > > Do we have any outside of intel_decode.c ? I'm failing
> > > > > > > > to spot any.
> > > > > > > 
> > > > > > > amdgpu_bo.c has 7 occurences of "= {}" and they all print
> > > > > > > the warning.
> > > > > > With 200+ cases of memset and 40+ of "= *{ *0 *}". Any
> > > > > > objections if I
> > > > > > send a patch to transition to either one of these two ?
> > > > > 
> > > > > That's up to you, but please note that I don't plan to stop
> > > > > using "= {}",
> > > > > because it's the most convenient way to clear memory in a lot
> > > > > of
> > > > > cases and takes only 4 bytes of text.
> > > > 
> > > > I like {} too and think we should encourage that. I'd rather
> > > > transition the { 0 } stuff over to {}.
> > > > 
> > > So people feel against seeing/writing single extra character 0,
> > > despite that the warning has helped catch actual bug ?
> > > And now are willing to transitions 40+ cases as opposed to ~15...
> > > that
> > > feels strange to say the least.
> > 
> > Does the '= { 0 }' thing even work if the first member happens to
> > be
> > something other than an integer?
> 
> No. That's why I like {}. Otherwise you end up doing
> {{{{{{{{{{0}}}}}}}}}.

ISO C99
According to 6.7.8 20 all braces but the outermost ones are optional.
{}, on the other hand, is not allowed by syntax rules.

Jan


> 
>   -ilia
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Emil Velikov Jan. 22, 2016, 7:21 p.m. UTC | #25
On 22 January 2016 at 18:49, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Jan 22, 2016 at 05:59:30PM +0000, Emil Velikov wrote:

>> Also let's not forget about
>>
>> a.c:17:20: warning: ISO C forbids empty initializer braces [-Wpedantic]
>>      struct foo f = {};
>>                     ^
>
> I long ago decided that -pedantic is stupid, hence I don't use it.
>
Only tried pedantic as I couldn't find any references to "= {}" in the
C spec. I'm not even remotely suggesting that we use it.

> My gcc (4.9.3 something) seems to allow the {0} but with a struct within
> a struct it angers -Wmissing-braces, although my reading of the spec
The -Wmissing-braces fix might get backported for 4.8 and 4.9 [1]

> suggests that it's pretty well defined how this sort of thing should
> behave. I was expecting some kind of 'implicit pointer from integer'
> warning when the thing it would initialize is a pointer, but didn't get
> one. Not sure why.
>
> And {} of course makes -Wmissing-field-initializers upset. I can't see
> anything in the spec to relly forbid this form, except that the syntax
> maybe doesn't allow for an empty initializer-list.
>
> About the only "useful" thing I learned from the spec is that 0 is an
> octal constant :) Makes some sense but it never occured to me before.
>
> So I guess all I can say is that gcc is stupid, and it should just stfu
> and let both '= {}' and '= {0}' through without whining about it.
>
Luckily with the 5 series things are shaping up :-)

Thanks for digging it up.
-Emil

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
Ville Syrjala Jan. 22, 2016, 7:33 p.m. UTC | #26
On Fri, Jan 22, 2016 at 07:21:31PM +0000, Emil Velikov wrote:
> On 22 January 2016 at 18:49, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jan 22, 2016 at 05:59:30PM +0000, Emil Velikov wrote:
> 
> >> Also let's not forget about
> >>
> >> a.c:17:20: warning: ISO C forbids empty initializer braces [-Wpedantic]
> >>      struct foo f = {};
> >>                     ^
> >
> > I long ago decided that -pedantic is stupid, hence I don't use it.
> >
> Only tried pedantic as I couldn't find any references to "= {}" in the
> C spec. I'm not even remotely suggesting that we use it.
> 
> > My gcc (4.9.3 something) seems to allow the {0} but with a struct within
> > a struct it angers -Wmissing-braces, although my reading of the spec
> The -Wmissing-braces fix might get backported for 4.8 and 4.9 [1]
> 
> > suggests that it's pretty well defined how this sort of thing should
> > behave. I was expecting some kind of 'implicit pointer from integer'
> > warning when the thing it would initialize is a pointer, but didn't get
> > one. Not sure why.
> >
> > And {} of course makes -Wmissing-field-initializers upset. I can't see
> > anything in the spec to relly forbid this form, except that the syntax
> > maybe doesn't allow for an empty initializer-list.
> >
> > About the only "useful" thing I learned from the spec is that 0 is an
> > octal constant :) Makes some sense but it never occured to me before.
> >
> > So I guess all I can say is that gcc is stupid, and it should just stfu
> > and let both '= {}' and '= {0}' through without whining about it.
> >
> Luckily with the 5 series things are shaping up :-)
> 
> Thanks for digging it up.
> -Emil
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119

Hmm. So yet another C vs. C++ difference. I guess everyone has given up
on the supposed ability of C++ to include C code.
diff mbox

Patch

diff --git a/configure.ac b/configure.ac
index c8c4ace..057a846 100644
--- a/configure.ac
+++ b/configure.ac
@@ -174,7 +174,8 @@  MAYBE_WARN="-Wall -Wextra \
 -Wstrict-aliasing=2 -Winit-self \
 -Wdeclaration-after-statement -Wold-style-definition \
 -Wno-unused-parameter \
--Wno-attributes -Wno-long-long -Winline -Wshadow"
+-Wno-attributes -Wno-long-long -Winline -Wshadow \
+-Wno-missing-field-initializers"
 
 # invalidate cached value if MAYBE_WARN has changed
 if test "x$libdrm_cv_warn_maybe" != "x$MAYBE_WARN"; then
diff --git a/intel/intel_decode.c b/intel/intel_decode.c
index e7aef74..287c342 100644
--- a/intel/intel_decode.c
+++ b/intel/intel_decode.c
@@ -38,8 +38,6 @@ 
 #include "intel_chipset.h"
 #include "intel_bufmgr.h"
 
-/* The compiler throws ~90 warnings. Do not spam the build, until we fix them. */
-#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
 
 /* Struct for tracking drm_intel_decode state. */
 struct drm_intel_decode {