[Mesa-dev] [PATCH] gallium: Use enum pipe_shader_type in bind_sampler_states()

[Brian Paul]

On 08/27/2016 11:18 AM, Jan Vesely wrote:
> On Fri, 2016-08-26 at 17:25 +0100, Eric Engestrom wrote:
>> On Fri, Aug 26, 2016 at 04:21:14PM +0200, Kai Wasserbäch wrote:
>>>
>>> Hey Eric,
>>> Eric Engestrom wrote on 26.08.2016 15:49:
>>>>
>>>> On Fri, Aug 26, 2016 at 03:14:57PM +0200, Kai Wasserbäch wrote:
>>>>>
>>>>> Brian Paul wrote on 26.08.2016 14:50:
>>>>>>
>>>>>> On 08/26/2016 05:58 AM, Kai Wasserbäch wrote:
>>>>>>>
>>>>>>> Cc: Brian Paul <brianp at vmware.com>
>>>>>>> Signed-off-by: Kai Wasserbäch <kai at dev.carbon-project.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Hi Brian,
>>>>>>> is this what you had in mind? If so, I was wondering
>>>>>>> whether virgl_encode.c
>>>>>>> would need to be updated as well. Doesn't seem like it,
>>>>>>> since the functions
>>>>>>> there map everything to uint32_t or some other standard
>>>>>>> type.
>>>>>>>
>>>>>>> Another point are the switch statements nouveau uses. To
>>>>>>> silence the -Wswitch
>>>>>>> warning of GCC I stuck a default case with two asserts at
>>>>>>> the end of them. But
>>>>>>> maybe it would be better to use an if...else for nv30 and
>>>>>>> nv50.
>>>>>> I think one assertion is enough.  It's up to the nouveau
>>>>>> developers whether they
>>>>>> want to do more.
>>>>> ok, then I'll go for the generic "unhandled type" assert.
>>>> I would've gone with `unreachable()`, since nothing outside the
>>>> enum range should ever get in.
>>> Do you feel strongly about that and require a v3?
>> I don't, it's fine as is.
>>
>>>
>>> Personally I think the
>>> assert() is better since the function could be called with another
>>> enum member
>>> value, which still is unhandled by the switch().
>> unreachable() still has an assert() (see src/util/macros.h:75)
>> The difference is that unreachable() tells the compiler that we won't
>> be
>> interested in what comes next. If we reach it, we abort with a
>> message
>> in debug builds, and after that the compiler can do whatever it
>> wants,
>> e.g. optimise out irrelevant code paths, and obviously it wont warn
>> about these code paths anymore, which is often more interesting.
>
> moreover, assert leaves untested codepath with silent failure in
> ndebug builds.
> In this case it would be better to use unreachable + unhandled enum
> values instead of default (so the compiler complains when new shader
> types get added, however unlikely that is).
>
> just my 2c,
> Jan

Kai, the series LGTM.  I'll add my R-b and push the four patches.

Do you plan to do more such as pipe_context::set_constant_buffer()

Also, git grep 'unsigned shader' and 'uint shader' shows quite a few 
more places where similar clean-ups are possible.  I'll probably take 
care of the VMware driver.

As for assert vs. unreachable, whichever people think is better is fine 
with me.  Off-hand, I'm not aware of any new types of shaders coming 
along any time soon so I think it's a minor issue which can be addressed 
in follow-on commits.

Thanks for doing this!

-Brian