[Mesa-dev] [RFC] intel/isl: Rewrite gen7_choose_image_alignment_el

[Kenneth Graunke]

On Wednesday, May 10, 2017 10:19:46 AM PDT Emil Velikov wrote:
> On 10 May 2017 at 16:38, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > On Wed, May 10, 2017 at 8:26 AM, Emil Velikov <emil.l.velikov at gmail.com>
> > wrote:
> >>
> >> Hi Jason,
> >>
> >> Humble unrelated question.
> >>
> >> On 9 May 2017 at 18:00, Jason Ekstrand <jason at jlekstrand.net> wrote:
> >>
> >> > +   if (isl_surf_usage_is_depth(info->usage)) {
> >> > +      if (info->format == ISL_FORMAT_R16_UNORM) {
> >> > +         return isl_extent3d(8, 4, 1);
> >> > +      } else {
> >> > +         return isl_extent3d(4, 4, 1);
> >> > +      }
> >> > +   } else if (isl_surf_usage_is_stencil(info->usage)) {
> >> > +      return isl_extent3d(8, 8, 1);
> >> > +   } else if (isl_format_is_compressed(info->format)) {
> >> > +      /* Compressed formats all have alignment equal to block size. */
> >> > +      return isl_extent3d(1, 1, 1);
> >> > +   }
> >>
> >> I've seen a handful of constructs like the above.. Is there any reason
> >> to keep the extra else/curly brackets?
> >> Something like the following reads a bit easier yet admittedly I'm not
> >> the person to set the coding style in isl.
> >
> >
> > Does it?
> >
> Indeed it does, I would not have bothered otherwise ;-)
> It's a subjective topic and my contributions here are, ahem, limited,
> so feel do ignore me.
> 
> Thanks
> Emil

I see Jason's point here - the depth/stencil/compressed blocks are
kind of like cases in a switch.  It's one of the three, and we're
choosing between them.  I think the code reads fine as is here.

That said, I almost always prefer Emil's style, FWIW.

   if (blah)
      return foo;

   return bar;

makes it obvious that something will be returned, as there's an
clear unconditional return at the top-level.  I especially like this
style when "foo" is for a special case, and "bar" is the general case.

   if (blah)
      return foo;
   else
      return bar;

is a little less clear IMO - you have to recognize that the control flow
in the inner block will happen, because one of the two blocks will
always be hit.

But it's largely a matter of taste, so...*shrug*.

If I were writing this, I would probably handle the R16 case as:

   if (info->format == ISL_FORMAT_R16_UNORM) {
      return isl_extent3d(8, 4, 1);

   return isl_extent3d(4, 4, 1);

but leave the rest as is.  Totally up to you though, Jason.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170510/14507a4b/attachment.sig>