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

[Francisco Jerez]

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.

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

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
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130824/fa7f1827/attachment.pgp>