On Wed 10 May 2017, Kenneth Graunke wrote: > On Wednesday, May 10, 2017 10:19:46 AM PDT Emil Velikov wrote: > > On 10 May 2017 at 16:38, Jason Ekstrand <ja...@jlekstrand.net> wrote: > > > On Wed, May 10, 2017 at 8:26 AM, Emil Velikov <emil.l.veli...@gmail.com> > > > wrote: > > >> > > >> Hi Jason, > > >> > > >> Humble unrelated question. > > >> > > >> On 9 May 2017 at 18:00, Jason Ekstrand <ja...@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.
It most definitely does :) > 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. I agree with Ken here. In a chain of early returns... if (x0) return r0; if (x1) return r1; return r2; ...the code flow is obvious at first glance by all readers, not just the author. The clarity, I believe, is due to two factors: - It resembles the natural human behavior of making choices. For example, when you're in grocery aisle trying to choose Ben & Jerry's, the thoughts usually proceed as below. Observe how similar it *feels* to read code in the early-return style to how it feels to choose the perfect pint of ice cream. - [Eyes dart to Strawberry Garcia] - Do I want strawberry today? ... - Nope. - [Eyes dart to The Tonight Dough] - Do I think Jimmy Fallon is funny? - Nope. - [Eyes dart to Boom Chocolatta Cookie Core] - Do I really want *that* much chocolate in my ice cream? - Nope. - [Eyes dart to Vanilla] - Have I been deceiving myself all these years? Am I, in truth, the predictable, boring chump that I have strove so hard to avoid becoming? Am I truly the type of person who, when confronted with a dizzying array of fanatastical ice cream wonders, plays it safe with vanilla, afraid of what may lie behind a decision too hasty, too uninformed, too bold? - Sigh. Yup. - [Grab the vanilla] - [Return home] // You've returned home! The following supermarket scenarios // are obviously no longer possible. - [Eyes dart to Peanut Butter World] ... - [Eyes dart to Pumpkin Cheesecake] ... - [Give up] - [Return home empty handed] - There is no oppurtunity for a "non-returning" branch to hide among the returning branches, like below. Readers (or, at least me) intuit this and feel more confidence that they understand the code. if (blah_blah_blah) { return r0; } else if (blah_foo_foo_blah) { do_stuff; } else if (oh_what_did_you_say) { return big_thing; } do_more_stuff(); return ok_now; > But it's largely a matter of taste, so...*shrug*. Not always. The Go code guidelines strongly recommend the "early return" style for error handling. The guide provides a good explanation. <https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow> > 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. Anyway, I've stated my preference. Either way, this patch is a large improvement over my original isl code. Reviewed-by: Chad Versace <chadvers...@chromium.org> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev