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

[Eric Engestrom]

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.

> 
> >>>> Cheers,
> >>>> Kai
> >>>>
> >>>> P.S.: If this is the right direction, I can do the remaining stuff as well.
> >>>
> >>> I think there was another person interested so feel free to share the task.
> >>
> >> Eric, I'm sorry I didn't see your message before I had prepared this patch and
> >> sent it. If I should leave the rest alone, just give me a shout. Would be
> >> unfortunate if we did the same task twice.
> > 
> > No worries, I also saw your message after having sent my reply on
> > Brian's comment, so I'm no better xD
> > 
> > I also have other ideas of things I could do, so I'll leave this
> > "int to enum" task to you :)
> 
> Ok, I'll prepare the other patches as well. (I'll probably sent them out tomorrow.)
> 
> Cheers,
> Kai
>