[PATCH] D22279: [ADT] Add FlagsEnum, used to enable bitwise operations on enums without static_cast.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: include/llvm/ADT/FlagsEnum.h:21
@@ +20,3 @@
+
+// ENABLE_FLAGS_ENUM lets you opt in an individual enum type so you can perform
+// bitwise operations on it without putting static_cast everywhere.
----------------
Doxygen?

Is "flags" more or less descriptive than "bitset"?

================
Comment at: include/llvm/ADT/FlagsEnum.h:39-41
@@ +38,5 @@
+//
+// ENABLE_FLAGS_ENUM must appear in the same namespace as the enum itself;
+// otherwise, the "using" statements inside the macro won't pull the operators
+// into the right namespace.
+#define ENABLE_FLAGS_ENUM(EnumType, LargestValue)                              \
----------------
This doesn't actually work AFAIK.

In C++11 we still have to specialize templates in the namespace they are declared.

So, either this needs to instead be two macros, one in the llvm namespace and one in the namespace of the enum, or we need something much, much more crafty.

I have a terrible idea. Instead of specializing a template, we can declare an overload of a function in the same namespace as the enum which accepts an enum as the argument and returns a reference to an array of characters the size of the largest value. Then you can call this function using unqualified lookup and ADL will find the one in the enum's namespace, and sizeof that call will tell you the largest value.

Too clever?

================
Comment at: include/llvm/ADT/FlagsEnum.h:61
@@ +60,3 @@
+  assert(Val >= 0 && "Negative enum values are not allowed.");
+  assert((NextPowerOf2(FlagsEnumTraits<E>::Largest) == 0 ||
+          Val < NextPowerOf2(FlagsEnumTraits<E>::Largest)) &&
----------------
Part of this can be a static_assert.

================
Comment at: include/llvm/ADT/FlagsEnum.h:62
@@ +61,3 @@
+  assert((NextPowerOf2(FlagsEnumTraits<E>::Largest) == 0 ||
+          Val < NextPowerOf2(FlagsEnumTraits<E>::Largest)) &&
+         "Enum value too large (or FlagsEnumTraits::Largest too small?)");
----------------
You could pre-compute the NextPowerOf2 bit as a constexpr and use it both in the static_assert above, here, and below. Maybe compute it as the mask and use <=?

================
Comment at: include/llvm/ADT/FlagsEnum.h:68
@@ +67,3 @@
+template <typename E, typename Dummy = typename std::enable_if<
+                          sizeof(FlagsEnumTraits<E>) >= 0>::type>
+E operator~(E Val) {
----------------
I would make two traits classes I think. One which is just a predicate and uses the idiomatic inheritance from true_type, and one which has the computed values etc.

I would also mildly prefer using std::enable_if on the return type rather than the template argument technique where we can. It ends up being a bit shorter IMO.


http://reviews.llvm.org/D22279