[Mesa-dev] [PATCH 2/2] svga: fix coverity MIXED_ENUMS warning

[Brian Paul]

On 05/31/2016 10:29 AM, Rob Clark wrote:
> On Tue, May 31, 2016 at 12:14 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 31.05.2016 um 16:33 schrieb Erik Faye-Lund:
>>> On Tue, May 31, 2016 at 3:46 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>> On Tue, May 31, 2016 at 9:29 AM, Brian Paul <brianp at vmware.com> wrote:
>>>>> On 05/31/2016 07:10 AM, Brian Paul wrote:
>>>>>>
>>>>>> On 05/29/2016 10:32 AM, Rob Clark wrote:
>>>>>>>
>>>>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>>>>
>>>>>>> Another pipe_resource_usage vs pipe_transfer_usage mixup.
>>>>>>>
>>>>>>> CID 1362169, 1362168
>>>>>>>
>>>>>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>>>>>> ---
>>>>>>>    src/gallium/drivers/svga/svga_resource_buffer.c | 2 +-
>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/src/gallium/drivers/svga/svga_resource_buffer.c
>>>>>>> b/src/gallium/drivers/svga/svga_resource_buffer.c
>>>>>>> index d91497c..77b35b3 100644
>>>>>>> --- a/src/gallium/drivers/svga/svga_resource_buffer.c
>>>>>>> +++ b/src/gallium/drivers/svga/svga_resource_buffer.c
>>>>>>> @@ -69,7 +69,7 @@ static void *
>>>>>>>    svga_buffer_transfer_map(struct pipe_context *pipe,
>>>>>>>                             struct pipe_resource *resource,
>>>>>>>                             unsigned level,
>>>>>>> -                         enum pipe_resource_usage usage,
>>>>>>> +                         enum pipe_transfer_usage usage,
>>>>>>>                             const struct pipe_box *box,
>>>>>>>                             struct pipe_transfer **ptransfer)
>>>>>>>    {
>>>>>>>
>>>>>>
>>>>>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>>>>>
>>>>>
>>>>> Actually, maybe that should be reverted to 'unsigned'.
>>>>>
>>>>> The parameter is actually a bitmask of the PIPE_TRANSFER_x flags.  We define
>>>>> those flags with the pipe_transfer_usage enum type.
>>>>>
>>>>> But, IIRC, some compilers complain about using enums as bitmasks.  I think
>>>>> this came up in the past but I don't recall the specifics.
>>>>
>>>> hmm, there are other places were we use enum bitmasks (like nir_variable_mode)..
>>>>
>>>> (that said, pretty much all I use is gcc so don't claim to know too
>>>> much about msvc, etc)
>>>
>>> I don't think I've seen MSVC complain about this in C code, but it's
>>> illegal in C++, AFAIK no matter what compiler.
>>>
>>> Just tested to make sure; this works fine in VS2015 in C ode but not
>>> int C++ code (the or promotes to int, and won't implicitly convert
>>> back).
>>>
>>> A common way to fix this, is to override the or operator. Here's an example:
>>>
>>> ---8<---
>>> enum my_enum {
>>>     my_foo = 1,
>>>     my_bar = 2
>>> };
>>>
>>> #ifdef __cplusplus
>>> inline my_enum operator|(my_enum a, my_enum b)
>>> {
>>>     return (my_enum)((int)a | (int)b);
>>> }
>>> #endif
>>>
>>> /* just a test to show that or'ing works as expected */
>>> enum my_enum test(enum my_enum a, enum my_enum b)
>>> {
>>>     return a | b;
>>> }
>>> ---8<---
>>
>> Isn't that a bit confusing in general using enums for bitmasks? I mean
>> where's the enumeration... Someone might see it's an enum and figure it
>> can only really be one of the enum values.
>
> I guess I always thought about it the opposite way..  enum params give
> a clear idea what value(s) are possible, and it is pretty obvious from
> the enum definition when things are bitmasks.  It is a lot easier to
> know what to pass to some fxn if the parameter is 'enum foo' than
> 'unsigned'..
>
> But only my $0.02..

I generally prefer enums.  In the past, I thought gdb had some issue 
with printing enum vars which were really bitmasks.  But I just did a 
quick test here and gdb 7.7 at least seems smart enough to actually 
figure out when an enum var's value a bitmask and will print "(FOO | BAR 
| ETC)" when that's the case.  Nice.

Sounds like C++ is still an issue though so we might have to be careful 
about this in a .h file that might be included by C++ code.

-Brian