[Mesa-dev] [PATCH 1/8] nir: Silence missing field initializer warnings for nir_src

[Ian Romanick]

On 12/14/2015 07:36 PM, Jason Ekstrand wrote:
> On Mon, Dec 14, 2015 at 5:12 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 12/14/2015 04:39 PM, Ilia Mirkin wrote:
>>> On Mon, Dec 14, 2015 at 7:28 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>> On 12/14/2015 03:38 PM, Ilia Mirkin wrote:
>>>>> It's a pretty standard feature of compilers to init things to 0 and
>>>>> not have the full structure specified like that... what compiler are
>>>>> you seeing these with? Can we just fix the glitch with a
>>>>> -Wno-stupid-warnings?
>>>>
>>>> I have observed this with several versions of GCC.
>>>>
>>>> In C, you can avoid this with a trailing comma like:
>>>>
>>>> #define NIR_SRC_INIT (nir_src) { { NULL }, }
>>>>
>>>> However, nir.h is also used in some C++ code where that doesn't help.
>>>>
>>>> To be honest, I'm not a big fan of these macros.  Without C99 designated
>>>> initalizers, maintaining initializers like these (or the ones in
>>>> src/glsl/builtin_variables.cpp) is a real pain.  We can't use those, and
>>>> we can't use C++ constructors.  We have no good options available. :(
>>>>
>>>> I thought about replacing them with a static inline function that
>>>> returns a zero-initialized struct.  The compiler should generate the
>>>> same code.  However, that doesn't work with uses like those in patch 3.
>>>>
>>>> I'm also a little curious why you didn't raise this issue when I sent
>>>> these patches out in August.  I removed the patch from the series that
>>>> you objected to back then.
>>>
>>> I have absolutely no recollection of any of that. Perhaps I saw "nir"
>>> and thought to myself, "don't care, let them do whatever, this won't
>>> ever affect me". Which is a sentiment I'm happy to continue with, by
>>> the way.
>>
>> Fair enough. :)  The patch I removed was one that removed the gl_context
>> parameter from a function in dd_function_table.
>>
>> http://patchwork.freedesktop.org/patch/58048/
>>
>>> I know that doing
>>>
>>> x = {}
>>>
>>> is a gcc extension, but I thought that {0} should always work (with
>>> enough {} nesting in case the first element is a struct). Perhaps it
>>
>> {0} is, basically what we're doing now, and GCC complains about it with
>> -Wmissing-field-initializers or -Wextra.  When we added C-style struct
> 
> I'm not a big fan of spending time fixing warnings that you have to
> add -Wextra to get.  However, if there are C++ issues, then those
> definitely need to get fixed.

Those options found real bugs in builtin_variables.cpp, and I'm a big
fan of that.

>> and array initializers to GLSL, we discussed adding this sort of
>> implicit zero initialization.  I did some digging in the C89 and C99
>> specs, and I have some recollection that in this case the missing fields
>> get undefined values... but, starting with C99, {0, } implicitly
>> initializes the missing fields to zero.  I also seem to recall that bit
>> of weirdness in C is why quite a few people were opposed to adding it to
>> GLSL.  This was several years ago, so my memory may not be completely
>> reliable.
>>
>>> doesn't in C++? I could believe that, although I'd be surprised.
>>
>> The initializer support in C++ intentionally quite a bit more primitive
>> than in C99.  The language designers want you to use constructors
>> whether it's the best tool for the job or not... which is why there are
>> no designated initializers.
> 
> So, I've got a patch somewhere that switches based on __cplusplus and
> defines NIR_SRC_INIT as either the C99 thing or nir_src() for C++.

I thought about doing something like that too.  Having to maintain and
keep in sync two separate versions of the initializer / constructor
doesn't sound like a maintainable solution either.  At best, it's the
kind of thing that I expect someone to see in a year, say "WTF?", and
submit a patch to change.

At worst, in a year we decide to add some field to nir_src that isn't
zero initialized, and we forget to update one of the initializers... and
end up with a hard to find bug.

> Would that solve this problem?  There was also a bug recently about us
> not building with oricle studio that it would probably fix.  If so,
> let's do that rather than a gigantic mess of braces and zeros.

We explicitly removed support for Oracle Studio, so that's not a
consideration.

> --Jason
> 
>>> Anyways, didn't mean to stir the pot too much, just thought there
>>> might be a simpler way out of all this.
>>
>> Well, there are. :) We just can't use them due to some combination of
>> MSVC, C++, and C99.
>>
>>> Cheers,
>>>
>>>   -ilia
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev