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

[Jason Ekstrand]

On Apr 19, 2016 6:10 PM, "Emil Velikov" <emil.l.velikov at gmail.com> wrote:
>
> 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.

I added more brackets. :-)

> So yes, it's annoyingly long/verbose solution but for a reason ;-)
> -Emil
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160419/ac03c025/attachment.html>