[Mesa-dev] [PATCH 1/4] linker: Trivial coding standards fixes

Ian Romanick idr at freedesktop.org
Wed Nov 9 17:25:53 UTC 2016


On 11/08/2016 05:25 AM, Ilia Mirkin wrote:
> On Tue, Nov 8, 2016 at 12:50 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> -   virtual void visit_field(const glsl_type *type, const char *name,
>> -                            bool row_major)
>> +   virtual void visit_field(const glsl_type *, const char *,
>> +                            bool /* row_major */)
>>     {
>> -      (void) type;
>> -      (void) name;
>> -      (void) row_major;
>> -      assert(!"Should not get here.");
>> +      unreachable("Should not get here.");
>>     }
> 
> I'd be in favor of leaving this as an assert. The unreachable gets you
> nothing here, except potential infinite loops on production builds
> should this path ever get hit somehow.

I don't think this will infinite loop.  This and the "fuller" parameter
version of visit_field are called at the leaves of the resource.

It looks like I added this version as a short-hand for users that didn't
need the fuller version.  I don't think there's any real utility in
that.  I'm not sure what my thinking was there.  Maybe if those users
overloaded the recursion function could just call the compact version to
avoid passing some parameters?  None of the users do that.

I'm going to submit a follow up patch that removes this overload.

> I think people have started going overboard with unreachable... it
> really should be for "shut up compiler, this can't happen, you're just
> too dumb to see it" cases. Not for "it would be a bug to hit this
> path" cases.

Yes and no.  It silences warnings, and it also gives the compiler
information.  We have observed that in some cases, especially the
"default: unreachable(...);" cases, the compiler will generate better
code with unreachable annotations.

Whether unreachable is different from assert is entirely dependent on
the build flags.  Is a production build one that lacks -DDEBUG, or is it
one that also has -DNDEBUG?  It depends on who's building.

>   -ilia



More information about the mesa-dev mailing list