On 08/24/2013 10:41 AM, Francisco Jerez wrote:
Chad Versace <chad.vers...@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 <curroje...@riseup.net>
For what it's worth, this patch is
Reviewed-by: Chad Versace <chad.vers...@linux.intel.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
