<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 19, 2016 at 8:12 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"><p dir="ltr"><br>
On Apr 19, 2016 6:10 PM, "Emil Velikov" <<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>> wrote:<br>
><br>
> On 19 April 2016 at 23:18, Chad Versace <<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>> wrote:<br>
> > On Tue 19 Apr 2016, Emil Velikov wrote:<br>
> >> On 16 April 2016 at 20:45, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br>
> >> > C++ doesn't support designated initializers and g++ in particular doesn't<br>
> >> > handle them when the struct gets complicated, i.e. has a union.<br>
> >> > ---<br>
> >> >  src/intel/isl/isl.h | 32 ++++++++++++++++++++------------<br>
> >> >  1 file changed, 20 insertions(+), 12 deletions(-)<br>
> >> ><br>
> >> > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h<br>
> >> > index 33d43d7..c3a1106 100644<br>
> >> > --- a/src/intel/isl/isl.h<br>
> >> > +++ b/src/intel/isl/isl.h<br>
> >> > @@ -988,25 +988,35 @@ isl_surf_info_is_z32_float(const struct isl_surf_init_info *info)<br>
> >> >  static inline struct isl_extent2d<br>
> >> >  isl_extent2d(uint32_t width, uint32_t height)<br>
> >> >  {<br>
> >> > -   return (struct isl_extent2d) { .w = width, .h = height };<br>
> >> > +   struct isl_extent2d e = {<br>
> >> > +      { width, },<br>
> >> > +      { height, },<br>
> >> > +   };<br>
> >> > +   return e;<br>
> >> >  }<br>
> >> ><br>
> >> Please use something like the following<br>
> >><br>
> >>    struct isl_extent2d e;<br>
> >><br>
> >>    memset(&e, 0, sizeof(e));<br>
> >>    e.w = width;<br>
> >>    e.h = height;<br>
> >><br>
> >>    return e;<br>
> >><br>
> >> It is a bit over expressive to write/look at, although it will give<br>
> >> you exactly what you want _all_ the time.<br>
> >> Otherwise you risk feeding garbage and/or setting the wrong struct<br>
> >> member as isl_extent2d change.<br>
> >><br>
> >> Hunting bugs that this may cause, is not what you want to do. Plus the<br>
> >> compiler will optimise it to the exact same code.<br>
> ><br>
> > I agree with Emil. Let's use explicit member names when initializing the<br>
> > structs. But, the memset isn't needed. The below initializaing pattern<br>
> > is more concise and *safer*, as it doesn't requiring typing the struct<br>
> > size.<br>
> ><br>
> >         struct isl_extent2d e = {0};<br>
> ><br>
> Almost... this won't play nice with C++ contexts. There's also the<br>
> fact that some C compilers will bark at us if we nest something within<br>
> the isl_extend2d struct, despite that the c99 standard (or was it c89)<br>
> is pretty clear that the above is sufficient.<br>
><br>
> Namely it will complain that we need another set of brackets. Check<br>
> out the GCC bug [1] about this.</p>
</div></div><p dir="ltr">I added more brackets. :-)</p></blockquote><div>FYI (for both of you), I've pushed the latest version of the series with your review feedback here:<br><br><a href="https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/image-load-store-no-gl-v3">https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/image-load-store-no-gl-v3</a><br><br></div><div>Sometimes it's hard just looking at patch discussions to see what the end result looks like.<br></div><div>--Jason<br></div><div> </div></div><br></div></div>