On 12/14/2015 07:36 PM, Jason Ekstrand wrote: > On Mon, Dec 14, 2015 at 5:12 PM, Ian Romanick <i...@freedesktop.org> wrote: >> On 12/14/2015 04:39 PM, Ilia Mirkin wrote: >>> On Mon, Dec 14, 2015 at 7:28 PM, Ian Romanick <i...@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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev