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

[Rob Clark]

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..

BR,
-R

> Roland
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev