[Mesa-dev] [PATCH 03/13] isl: Don't use designated initializers in the header

[Emil Velikov]

On 19 April 2016 at 23:18, Chad Versace <chad.versace at intel.com> wrote:
> On Tue 19 Apr 2016, Emil Velikov wrote:
>> On 16 April 2016 at 20:45, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> > C++ doesn't support designated initializers and g++ in particular doesn't
>> > handle them when the struct gets complicated, i.e. has a union.
>> > ---
>> >  src/intel/isl/isl.h | 32 ++++++++++++++++++++------------
>> >  1 file changed, 20 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
>> > index 33d43d7..c3a1106 100644
>> > --- a/src/intel/isl/isl.h
>> > +++ b/src/intel/isl/isl.h
>> > @@ -988,25 +988,35 @@ isl_surf_info_is_z32_float(const struct isl_surf_init_info *info)
>> >  static inline struct isl_extent2d
>> >  isl_extent2d(uint32_t width, uint32_t height)
>> >  {
>> > -   return (struct isl_extent2d) { .w = width, .h = height };
>> > +   struct isl_extent2d e = {
>> > +      { width, },
>> > +      { height, },
>> > +   };
>> > +   return e;
>> >  }
>> >
>> Please use something like the following
>>
>>    struct isl_extent2d e;
>>
>>    memset(&e, 0, sizeof(e));
>>    e.w = width;
>>    e.h = height;
>>
>>    return e;
>>
>> It is a bit over expressive to write/look at, although it will give
>> you exactly what you want _all_ the time.
>> Otherwise you risk feeding garbage and/or setting the wrong struct
>> member as isl_extent2d change.
>>
>> Hunting bugs that this may cause, is not what you want to do. Plus the
>> compiler will optimise it to the exact same code.
>
> I agree with Emil. Let's use explicit member names when initializing the
> structs. But, the memset isn't needed. The below initializaing pattern
> is more concise and *safer*, as it doesn't requiring typing the struct
> size.
>
>         struct isl_extent2d e = {0};
>
Almost... this won't play nice with C++ contexts. There's also the
fact that some C compilers will bark at us if we nest something within
the isl_extend2d struct, despite that the c99 standard (or was it c89)
is pretty clear that the above is sufficient.

Namely it will complain that we need another set of brackets. Check
out the GCC bug [1] about this.

So yes, it's annoyingly long/verbose solution but for a reason ;-)
-Emil

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119