Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
Francisco Jerez curroje...@riseup.net writes:
Ian Romanick i...@freedesktop.org writes:
On 08/26/2013 01:10 PM, Francisco Jerez wrote:
[...]
The thing is that defining bitwise operators separately for each enum
type, as this patch and the macro solution do, doesn't stop the compiler
From using the corresponding built-in integer operators when it doesn't
find a match among the user-defined ones. That means that if we have
two bitfield enumerants from two different disjoint types
e.g. SEASON_OF_THE_YEAR_SUMMER and a CPU_ARCHITECTURE_I386, the
compiler is still going to accept expressions like
SEASON_OF_THE_YEAR_SUMMER | CPU_ARCHITECTURE_I386, which might not be
what had been expected if the BRW_CXX_ENUM_OPS macro was used in an
attempt to improve the code's type safety.
This sounds insane.
If there are no operator overloads, the compiler rejects:
enum foo f(enum foo a, enum foo b)
{
return a | b;
}
Here both a and b would be implicitly converted to integers which are
then or'ed together using the built-in '|' operator yielding an integer.
Using the result as return value of your enum foo function fails
because in C++ integers are not implicitly convertible to enum types.
Then we add operloads:
enum foo operator|(enum foo, enum foo);
enum bar operator|(enum bar, enum bar);
And now the compiler will accept:
unsigned f(enum foo a, enum bar b)
{
return a | b;
}
That's accepted because both a and b are converted to integers as
before, giving an integer as result, which can be implicitly converted
to your unsigned return type just fine. It's easy to avoid that though
by defining a general overload:
templatetypename T, typename S T operator|(T, S);
for any bitfield types T and S that includes a static assertion
within its definition to make sure that both types are indeed the same.
In your example above the compiler would find an exact match during
overload resolution (our operator|foo, bar(foo, bar);) so it
wouldn't have to bother with implicit argument conversions. The
instantiation of that template would fail thanks to the static assertion
requiring both T and S to be equal.
I hope this makes the idea a bit clearer.
That can't be right. Am I missing something? Or am I reinforcing my
point about not being ready for this level of C++ ninjitsu...
This is way overcomplicated. I think we're doing just fine with the
status quo of #defines for our bitfields with namespaced names.
pgptvgKWfB3KF.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
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:
| templatetypename T
| struct bitmask_enumeration_traits;
|
| struct __empty_bitmask {
|templatetypename T
|operator T() const {
| bitmask_enumeration_traitsT();
| 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_traitsE {};
|
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
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
Ian Romanick i...@freedesktop.org writes: [...] 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. [...] 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. My explanation is exactly as relevant if we stick to Chad's solution or not, using macros doesn't save you from getting the unexpected effect I was trying to describe -- quite the opposite, I can't think of any simple way to work around that problem without using templates. The thing is that defining bitwise operators separately for each enum type, as this patch and the macro solution do, doesn't stop the compiler From using the corresponding built-in integer operators when it doesn't find a match among the user-defined ones. That means that if we have two bitfield enumerants from two different disjoint types e.g. SEASON_OF_THE_YEAR_SUMMER and a CPU_ARCHITECTURE_I386, the compiler is still going to accept expressions like SEASON_OF_THE_YEAR_SUMMER | CPU_ARCHITECTURE_I386, which might not be what had been expected if the BRW_CXX_ENUM_OPS macro was used in an attempt to improve the code's type safety. The template-based solution might seem somewhat obscure to the inexperienced C++ programmer, but unlike the macro-based one it would be semantically sound, and this is just one of many reasons why it's a good idea for anyone dealing with C++ code to have at least some basic knowledge on using templates -- it's the cleanest way to do static polymorphism and generic programming in C++. That said, I agree that it would be a bad idea to make a sudden transition to template metaprogramming in components of our tree where the majority of maintainers have a strong preference towards old-school C programming, but that's no reason to reject a small, non-intrusive and potentially useful change in the good direction a priori before having seen any of the code... [...] pgpAlZKw5443E.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
On 08/26/2013 01:10 PM, Francisco Jerez wrote:
Ian Romanick i...@freedesktop.org writes:
[...]
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.
[...]
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.
My explanation is exactly as relevant if we stick to Chad's solution or
not, using macros doesn't save you from getting the unexpected effect I
was trying to describe -- quite the opposite, I can't think of any
simple way to work around that problem without using templates.
The thing is that defining bitwise operators separately for each enum
type, as this patch and the macro solution do, doesn't stop the compiler
From using the corresponding built-in integer operators when it doesn't
find a match among the user-defined ones. That means that if we have
two bitfield enumerants from two different disjoint types
e.g. SEASON_OF_THE_YEAR_SUMMER and a CPU_ARCHITECTURE_I386, the
compiler is still going to accept expressions like
SEASON_OF_THE_YEAR_SUMMER | CPU_ARCHITECTURE_I386, which might not be
what had been expected if the BRW_CXX_ENUM_OPS macro was used in an
attempt to improve the code's type safety.
This sounds insane.
If there are no operator overloads, the compiler rejects:
enum foo f(enum foo a, enum foo b)
{
return a | b;
}
Then we add operloads:
enum foo operator|(enum foo, enum foo);
enum bar operator|(enum bar, enum bar);
And now the compiler will accept:
unsigned f(enum foo a, enum bar b)
{
return a | b;
}
That can't be right. Am I missing something? Or am I reinforcing my
point about not being ready for this level of C++ ninjitsu...
The template-based solution might seem somewhat obscure to the
inexperienced C++ programmer, but unlike the macro-based one it would be
semantically sound, and this is just one of many reasons why it's a good
idea for anyone dealing with C++ code to have at least some basic
knowledge on using templates -- it's the cleanest way to do static
polymorphism and generic programming in C++.
That said, I agree that it would be a bad idea to make a sudden
transition to template metaprogramming in components of our tree where
the majority of maintainers have a strong preference towards old-school
C programming, but that's no reason to reject a small, non-intrusive and
potentially useful change in the good direction a priori before having
seen any of the code...
[...]
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
Ian Romanick i...@freedesktop.org writes:
On 08/26/2013 01:10 PM, Francisco Jerez wrote:
[...]
The thing is that defining bitwise operators separately for each enum
type, as this patch and the macro solution do, doesn't stop the compiler
From using the corresponding built-in integer operators when it doesn't
find a match among the user-defined ones. That means that if we have
two bitfield enumerants from two different disjoint types
e.g. SEASON_OF_THE_YEAR_SUMMER and a CPU_ARCHITECTURE_I386, the
compiler is still going to accept expressions like
SEASON_OF_THE_YEAR_SUMMER | CPU_ARCHITECTURE_I386, which might not be
what had been expected if the BRW_CXX_ENUM_OPS macro was used in an
attempt to improve the code's type safety.
This sounds insane.
If there are no operator overloads, the compiler rejects:
enum foo f(enum foo a, enum foo b)
{
return a | b;
}
Here both a and b would be implicitly converted to integers which are
then or'ed together using the built-in '|' operator yielding an integer.
Using the result as return value of your enum foo function fails
because in C++ integers are not implicitly convertible to enum types.
Then we add operloads:
enum foo operator|(enum foo, enum foo);
enum bar operator|(enum bar, enum bar);
And now the compiler will accept:
unsigned f(enum foo a, enum bar b)
{
return a | b;
}
That's accepted because both a and b are converted to integers as
before, giving an integer as result, which can be implicitly converted
to your unsigned return type just fine. It's easy to avoid that though
by defining a general overload:
templatetypename T, typename S T operator|(T, S);
for any bitfield types T and S that includes a static assertion
within its definition to make sure that both types are indeed the same.
In your example above the compiler would find an exact match during
overload resolution (our operator|foo, bar(foo, bar);) so it
wouldn't have to bother with implicit argument conversions. The
instantiation of that template would fail thanks to the static assertion
requiring both T and S to be equal.
I hope this makes the idea a bit clearer.
That can't be right. Am I missing something? Or am I reinforcing my
point about not being ready for this level of C++ ninjitsu...
pgpZ0yWkFdgbH.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
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:
| templatetypename T
| struct bitmask_enumeration_traits;
|
| struct __empty_bitmask {
|templatetypename T
|operator T() const {
| bitmask_enumeration_traitsT();
| 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_traitsE {};
|
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 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
pgpjfqDaVRvuS.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
(From a suggestion by Francisco Jerez)
If an enum represents a bitfield of flags, e.g.:
enum E {
A = 1,
B = 2,
C = 4,
D = 8,
};
then C++ normally prohibits statements like this:
enum E x = A | B;
because A and B are implicitly converted to ints before OR-ing them,
and an int can't be stored in an enum without a type cast. C, on the
other hand, allows an int to be implicitly converted to an enum
without casting.
In the past we've dealt with this situation by storing flag bitfields
as ints. This avoids ugly casting at the expense of some type safety
that C++ would normally have offered (e.g. we get no warning if we
accidentally use the wrong enum type).
However, we can get the best of both worlds if we override the |
operator. The ugly casting is confined to the operator overload, and
we still get the benefit of C++ making sure we don't use the wrong
enum type.
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.
So what do folks think? Is it worth it?
Cc: Francisco Jerez curroje...@riseup.net
---
src/mesa/drivers/dri/i965/brw_clip.h | 2 +-
src/mesa/drivers/dri/i965/brw_clip_util.c | 2 +-
src/mesa/drivers/dri/i965/brw_eu.h | 16 +++-
src/mesa/drivers/dri/i965/brw_eu_emit.c| 4 ++--
src/mesa/drivers/dri/i965/brw_sf_emit.c| 12
src/mesa/drivers/dri/i965/brw_vec4.h | 2 +-
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 3 ++-
7 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
b/src/mesa/drivers/dri/i965/brw_clip.h
index 5af0ad3..41f5c75 100644
--- a/src/mesa/drivers/dri/i965/brw_clip.h
+++ b/src/mesa/drivers/dri/i965/brw_clip.h
@@ -173,7 +173,7 @@ void brw_clip_init_planes( struct brw_clip_compile *c );
void brw_clip_emit_vue(struct brw_clip_compile *c,
struct brw_indirect vert,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint header);
void brw_clip_kill_thread(struct brw_clip_compile *c);
diff --git a/src/mesa/drivers/dri/i965/brw_clip_util.c
b/src/mesa/drivers/dri/i965/brw_clip_util.c
index d5c50d7..24d053e 100644
--- a/src/mesa/drivers/dri/i965/brw_clip_util.c
+++ b/src/mesa/drivers/dri/i965/brw_clip_util.c
@@ -313,7 +313,7 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c,
void brw_clip_emit_vue(struct brw_clip_compile *c,
struct brw_indirect vert,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint header)
{
struct brw_compile *p = c-func;
diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
b/src/mesa/drivers/dri/i965/brw_eu.h
index 9053ea2..069b223 100644
--- a/src/mesa/drivers/dri/i965/brw_eu.h
+++ b/src/mesa/drivers/dri/i965/brw_eu.h
@@ -229,6 +229,8 @@ void brw_set_dp_write_message(struct brw_compile *p,
GLuint send_commit_msg);
enum brw_urb_write_flags {
+ BRW_URB_WRITE_NO_FLAGS = 0,
+
/**
* Causes a new URB entry to be allocated, and its address stored in the
* destination register (gen 7).
@@ -271,11 +273,23 @@ enum brw_urb_write_flags {
BRW_URB_WRITE_ALLOCATE | BRW_URB_WRITE_COMPLETE,
};
+#ifdef __cplusplus
+/**
+ * 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
+
void brw_urb_WRITE(struct brw_compile *p,
struct brw_reg dest,
GLuint msg_reg_nr,
struct brw_reg src0,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint msg_length,
GLuint response_length,
GLuint offset,
diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
b/src/mesa/drivers/dri/i965/brw_eu_emit.c
index b55b57e..ecf8597 100644
--- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
+++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
@@ -515,7 +515,7 @@ static void brw_set_ff_sync_message(struct brw_compile *p,
static void brw_set_urb_message( struct brw_compile *p,
struct brw_instruction *insn,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint msg_length,
GLuint response_length,
GLuint offset,
@@ -2213,7 +2213,7 @@ void brw_urb_WRITE(struct brw_compile *p,
struct brw_reg dest,
GLuint msg_reg_nr,
struct brw_reg src0,
- unsigned flags,
+
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
This is a nice improvement over the explicit cast, which is how I've always
done it in the past - which is the ugly part of an otherwise great method
for flags. Also I use a lot with enum for clearing bits.
On Fri, Aug 23, 2013 at 3:18 PM, Paul Berry stereotype...@gmail.com wrote:
(From a suggestion by Francisco Jerez)
If an enum represents a bitfield of flags, e.g.:
enum E {
A = 1,
B = 2,
C = 4,
D = 8,
};
then C++ normally prohibits statements like this:
enum E x = A | B;
because A and B are implicitly converted to ints before OR-ing them,
and an int can't be stored in an enum without a type cast. C, on the
other hand, allows an int to be implicitly converted to an enum
without casting.
In the past we've dealt with this situation by storing flag bitfields
as ints. This avoids ugly casting at the expense of some type safety
that C++ would normally have offered (e.g. we get no warning if we
accidentally use the wrong enum type).
However, we can get the best of both worlds if we override the |
operator. The ugly casting is confined to the operator overload, and
we still get the benefit of C++ making sure we don't use the wrong
enum type.
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.
So what do folks think? Is it worth it?
Cc: Francisco Jerez curroje...@riseup.net
---
src/mesa/drivers/dri/i965/brw_clip.h | 2 +-
src/mesa/drivers/dri/i965/brw_clip_util.c | 2 +-
src/mesa/drivers/dri/i965/brw_eu.h | 16 +++-
src/mesa/drivers/dri/i965/brw_eu_emit.c| 4 ++--
src/mesa/drivers/dri/i965/brw_sf_emit.c| 12
src/mesa/drivers/dri/i965/brw_vec4.h | 2 +-
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 3 ++-
7 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
b/src/mesa/drivers/dri/i965/brw_clip.h
index 5af0ad3..41f5c75 100644
--- a/src/mesa/drivers/dri/i965/brw_clip.h
+++ b/src/mesa/drivers/dri/i965/brw_clip.h
@@ -173,7 +173,7 @@ void brw_clip_init_planes( struct brw_clip_compile *c
);
void brw_clip_emit_vue(struct brw_clip_compile *c,
struct brw_indirect vert,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint header);
void brw_clip_kill_thread(struct brw_clip_compile *c);
diff --git a/src/mesa/drivers/dri/i965/brw_clip_util.c
b/src/mesa/drivers/dri/i965/brw_clip_util.c
index d5c50d7..24d053e 100644
--- a/src/mesa/drivers/dri/i965/brw_clip_util.c
+++ b/src/mesa/drivers/dri/i965/brw_clip_util.c
@@ -313,7 +313,7 @@ void brw_clip_interp_vertex( struct brw_clip_compile
*c,
void brw_clip_emit_vue(struct brw_clip_compile *c,
struct brw_indirect vert,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint header)
{
struct brw_compile *p = c-func;
diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
b/src/mesa/drivers/dri/i965/brw_eu.h
index 9053ea2..069b223 100644
--- a/src/mesa/drivers/dri/i965/brw_eu.h
+++ b/src/mesa/drivers/dri/i965/brw_eu.h
@@ -229,6 +229,8 @@ void brw_set_dp_write_message(struct brw_compile *p,
GLuint send_commit_msg);
enum brw_urb_write_flags {
+ BRW_URB_WRITE_NO_FLAGS = 0,
+
/**
* Causes a new URB entry to be allocated, and its address stored in
the
* destination register (gen 7).
@@ -271,11 +273,23 @@ enum brw_urb_write_flags {
BRW_URB_WRITE_ALLOCATE | BRW_URB_WRITE_COMPLETE,
};
+#ifdef __cplusplus
+/**
+ * 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
+
void brw_urb_WRITE(struct brw_compile *p,
struct brw_reg dest,
GLuint msg_reg_nr,
struct brw_reg src0,
- unsigned flags,
+ enum brw_urb_write_flags flags,
GLuint msg_length,
GLuint response_length,
GLuint offset,
diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
b/src/mesa/drivers/dri/i965/brw_eu_emit.c
index b55b57e..ecf8597 100644
--- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
+++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
@@ -515,7 +515,7 @@ static void brw_set_ff_sync_message(struct brw_compile
*p,
static void brw_set_urb_message( struct brw_compile *p,
struct brw_instruction *insn,
- unsigned flags,
+ enum brw_urb_write_flags flags,
Re: [Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
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.
Disadvantage (b) can be made painless with the macro I discuss below.
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.
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
