On 19 June 2017 at 20:52, Chad Versace <chadvers...@chromium.org> wrote:
> On Fri 16 Jun 2017, Christian Gmeiner wrote:
>> 2017-06-16 14:54 GMT+02:00 Emil Velikov <emil.l.veli...@gmail.com>:
>> > On 15 June 2017 at 21:47, Robert Foss <robert.f...@collabora.com> wrote:
>> >> From: Tomeu Vizoso <tomeu.viz...@collabora.com>
>> >>
>> >> Signed-off-by: Robert Foss <robert.f...@collabora.com>
>> >> ---
>> >> src/gallium/drivers/etnaviv/etnaviv_compiler.c | 2 ++
>> >> 1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/src/gallium/drivers/etnaviv/etnaviv_compiler.c
>> >> b/src/gallium/drivers/etnaviv/etnaviv_compiler.c
>> >> index eafb511bb8..8f73113059 100644
>> >> --- a/src/gallium/drivers/etnaviv/etnaviv_compiler.c
>> >> +++ b/src/gallium/drivers/etnaviv/etnaviv_compiler.c
>> >> @@ -885,6 +885,8 @@ etna_amode(struct tgsi_ind_register indirect)
>> >> default:
>> >> assert(!"Invalid swizzle");
>> >> }
>> >> +
>> >> + return 0;
>> > All the cases are handled correctly and even the default one will
>> > never be reached.
>> > Guess the compiler isn't smart enough, since we're using int:2 while
>> > in reality we're storing an enum and using enum:2 might not always
>> > work.
>> >
>> > Alternative solutions is s/assert/unreachable/. The call will be up-to
>> > the etna devs, but including something like the above (in the commit
>> > summary or elsewhere) will be a good idea, IMHO.
>> >
>>
>> I really like Emil's idea of s/assert/unreachable/
>
> If you replace assert() with unreachable() for an exhaustive switch
> statement, it's best to delete the default case and call unreachable()
> *after* the switch statement. That way, if new enums are ever added, the
> compiler will emit a -Wswitch warning (which would be otherwise
> supressed by the default case); someone will hopefully notice the
> warning, fix it, and save users from an out-of-bounds instruction
> pointer.
That's a very good idea, I think various parts of Gallium already use it.
To do that one needs to tweak struct tgsi_ind_register first - see below.
IIRC some versions of GCC have been having issues with explicitly
sized enums, although we already use it elsewhere in Mesa so it's not
a deal breaker.
struct tgsi_ind_register
{
...
- unsigned Swizzle : 2; /* TGSI_SWIZZLE_ */
+ enum tgsi_swizzle Swizzle : 2; /* TGSI_SWIZZLE_ */
...
}
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
