[Mesa-dev] [PATCH] mesa: Define helper function to get the number of texture layers.

[Matt Turner]

On Tue, Dec 10, 2013 at 1:31 PM, Brian Paul <brianp at vmware.com> wrote:
> On 12/10/2013 11:06 AM, Francisco Jerez wrote:
>>
>> Paul Berry <stereotype441 at gmail.com> writes:
>>
>>> On 10 December 2013 08:42, Francisco Jerez <currojerez at riseup.net> wrote:
>>>
>>>> Brian Paul <brianp at vmware.com> writes:
>>>>
>>>>> On 12/07/2013 06:17 PM, Francisco Jerez wrote:
>>>>>>
>>>>>> [...]
>>>>>> +   default:
>>>>>> +      unreachable();
>>>>>
>>>>>
>>>>> I think I'd like to see an assertion or _mesa_problem() call to catch
>>>>> unhandled cases if new texture targets are added in the future.
>>>>>
>>>>>
>>>> How about having the unreachable() macro print out an error and abort if
>>>> it's ever reached?  See the attached patch.
>>>>
>>>
>>> I would prefer for us to simply do this:
>>>
>>> assert(!"Unexpected texture target");
>>> unreachable();
>>>
>>> That would have the exact same semantics as your proposed patch (assert
>>> in
>>> debug builds, allow compiler optimizations in release builds), and it
>>> would
>>> have the added advantage that it's obvious what's going on to someone
>>> reading the code.  With your proposed patch it's not obvious--the reader
>>> has to be aware that Mesa has a non-standard meaning for unreachable().
>>
>>
>> The message adds no additional useful information IMHO, and it's already
>> obvious for anyone reading the code that something marked with
>> unreachable() shouldn't ever be reached -- What else do we have to say?
>> I have the impression that using the assert(!"") idiom before every
>> occurrence of unreachable() just increases the amount of noise in the
>> code and the likelihood of making mistakes.
>>
>> Regardless of what we do in this specific case, I believe that making
>> the unreachable() macro abort is a good thing because it will catch
>> cases where we have forgotten to pair it with an assert call [I have
>> already, a few times], and mistakes like writing 'assert("!...' [see
>> 61143b87c16231a2df0d69324d531503027f9aca].
>>
>> If you would like to include some additional explanation in an
>> unreachable block you can just write a comment, or make unreachable() a
>> variadic macro that prints its (optionally passed) argument to stderr
>> when the unreachable block is executed.
>
>
>
> I just did a test.  I put unreachable() in a conspicuous place in Mesa and
> ran glxinfo.  I got a segfault.  And in gdb the stack trace wasn't even
> correct.
>
> An assert would be much more useful since it tells us the location and we
> can get a good stack trace in gdb.
>
> The gcc docs for __builtin_unreachable say it's about expressing your intent
> when doing unconventional flow-control, and possibly suppressing compiler
> warnings.  It doesn't sounds like a debug check to report when there's
> unexpected control flow.
>
> I'm more convinced now that we should simply have an assert there rather
> than unreachable().

I think we want both. The assert lets us know in debug mode that
something is badly wrong, and the unreachable tells the compiler that
this should never be reached and to not warn in release builds.

I agree that using only unreachable is bad. The two existing uses of
it (I added both) are guarded by asserts as well.

I don't particularly like the idea of modifying unreachable to take a
string to print.