On Tue, Apr 19, 2016 at 8:12 PM, Jason Ekstrand <ja...@jlekstrand.net>
wrote:
>
> On Apr 19, 2016 6:10 PM, "Emil Velikov" <emil.l.veli...@gmail.com> wrote:
> >
> > On 19 April 2016 at 23:18, Chad Versace <chad.vers...@intel.com> wrote:
> > > On Tue 19 Apr 2016, Emil Velikov wrote:
> > >> On 16 April 2016 at 20:45, Jason Ekstrand <ja...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
