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

[Jan Vesely]

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

> 
> > 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 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
> > 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160827/1f3a2a52/attachment.sig>