[Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.

[Ian Romanick]

On 08/24/2013 10:41 AM, Francisco Jerez wrote:
> Chad Versace <chad.versace at linux.intel.com> writes:
>
>> On 08/23/2013 02:18 PM, Paul Berry wrote:
>>
>>> The disadvantages are that (a) we need an explicit enum value for 0,
>>> and (b) we can't use related operators like |= unless we define
>>> additional overloads.
>>
>> Disadvantage (a) is trivial, not really a problem.
>>
> I agree, it's not a real disadvantage, you can always define an "empty"
> enumerant that evaluates to zero.  If that seems inconvenient or ugly we
> can define a global constant that may be implicitly converted to any
> bitmask-like enumeration type evaluating to zero, as in:
>
> | template<typename T>
> | struct bitmask_enumeration_traits;
> |
> | struct __empty_bitmask {
> |    template<typename T>
> |    operator T() const {
> |       bitmask_enumeration_traits<T>();
> |       return T();
> |    }
> |
> |    operator unsigned() const {
> |       return 0;
> |    }
> | };
> |
> | const __empty_bitmask empty;
>
> The "bitmask_numeration_traits" structure makes sure that the
> polymorphic conversion operator won't be instantiated for non-bitmask
> types.  The enum declaration would look like:
>
> | enum E {
> |  A = 1,
> |  B = 2,
> |  C = 4,
> |  D = 8
> | };
> |
> | template<>
> | struct bitmask_enumeration_traits<E> {};
> |
>
> Actually, it *is* possible to arrange for the literal 0 to be
> implicitly converted to our bitmask enum types in a type-safe way by
> exploiting the fact that the language allows it to be implicitly
> converted to any pointer type (while implicit conversion of any other
> integral expression to a pointer type is disallowed).  Though I believe
> it would involve using an actual object instead of plain enums because
> it's not possible to define a conversion constructor for an enum type.
>
>> Disadvantage (b) can be made painless with the macro I discuss below.
>>
>>
> IMHO it would be nicer to define generic templates implementing
> overloads for all bitwise operators.  They would have to reference the
> bitmask_enumeration_traits structure so they would be discarded for
> non-bitmask types.
>
> Aside from being nicer and safer it would have two clear advantages.
> First, if we decide to use the "__empty_bitmask" type defined above, we
> wouldn't have to keep around three different overloads for each binary
> operator (T op T, empty op T, T op empty), we'd just define a general
> one 'T op S' that would be discarded by the SFINAE rule for incompatible
> T and S.
>
> Second, we could arrange for the expression 'FOO op BAR' (with 'FOO' and
> 'BAR' enumerants from different incompatible bitmask types) to be
> rejected by the compiler by means of a static assertion in the
> definition of 'T op S'.  If we use the macro solution below the compiler
> will accept that expression by downgrading both T and S to integers and
> then applying the built-in definition of 'op'.  Though it would still
> refuse to assign the result to a variable of any of both types *if* the
> user is doing that.

As a non-C++ programmer, that explanation gave me a headache.  I don't 
think this project is ready yet for its developers to need that level of 
knowledge of the C++ type system.

I can immediately understand Chad's macro, and I can also (nearly 
immediately) understand that it's probably not the "C++ way."

>>> So what do folks think?  Is it worth it?
>>
>>
>> Yes, I think it's worth it. The code becomes more readable and
>> more type safe, as evidenced by the diff lines like this:
>>> -                       unsigned flags,
>>> +                       enum brw_urb_write_flags flags,
>>
>> If we continue to do this to other enums, then we should probably
>> define a convenience macro to define all the needed overloaded
>> bit operators. Like this:
>>
>>    #define BRW_CXX_ENUM_OPS(type_name) \
>>        static inline type_name \
>>        operator|(type_name x, type_name y) \
>>        { \
>>           return (type_name) ((int) x | (int) y); \
>>        } \
>>        \
>>        static inline type_name \
>>        operator&(........) \
>>        ........ and more operators
>>
>>
>>> +/**
>>> + * Allow brw_urb_write_flags enums to be ORed together (normally C++ wouldn't
>>> + * allow this without a type cast).
>>> + */
>>> +inline enum brw_urb_write_flags
>>> +operator|(enum brw_urb_write_flags x, enum brw_urb_write_flags y)
>>> +{
>>> +   return (enum brw_urb_write_flags) ((int) x | (int) y);
>>> +}
>>> +#endif
>>
>> I think the comment is distracting rather than helpful. There is no need for C++
>> code to apologize for being C++ code.

You keep forgetting that a number of the people on this project are not 
C++ programmers.  Every one of them will ask, "Why is this necessary?" 
Comments are for communicating with humans.

> I agree, the comment seems redundant to me (as well as using the 'enum'
> keyword before enum names, though that's just a matter of taste).  In a

Again, this communicates to other humans what the thing is.  If I just 
see "foo x;" as a declaration, I don't know what kind of a thing (class, 
POD struct, enum, typedef, etc.) x is.  While the compiler doesn't care, 
humans do.

> general definition you might want to use the static_cast<> operator
> instead of a c-style cast, to make clear you're only interested in
> "safe" integer-to-enum casts.
>
> In any case this seems like a good thing to do, already in this state,
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>
>> For what it's worth, this patch is
>> Reviewed-by: Chad Versace <chad.versace at linux.intel.com>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev