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

[Jason Ekstrand]

On Tue, Apr 19, 2016 at 8:12 PM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

>
> 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. :-)
>
FYI (for both of you), I've pushed the latest version of the series with
your review feedback here:

https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/image-load-store-no-gl-v3

Sometimes it's hard just looking at patch discussions to see what the end
result looks like.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160419/732bceae/attachment-0001.html>