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

[Justin Lebar via llvm-commits]

jlebar 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.
----------------
chandlerc wrote:
> Doxygen?
> 
> Is "flags" more or less descriptive than "bitset"?
I considered this, but preferred "flags" over "bitset", because "bitset" sounds to me like a class that's like vector<bool>, where I can set bit N, where N is a runtime variable.  Not unlike std::bitset.

I added some doxygen bits.  Wanted to check to see how it's rendered, but it's spun 20 minutes so far, so I now have difficulty caring...

================
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)                              \
----------------
chandlerc wrote:
> 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?
I like the idea; it's pretty clean, aside from requiring a long name on the function we declare (since we'll be polluting others' namespaces with it).

================
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)) &&
----------------
chandlerc wrote:
> Part of this can be a static_assert.
Beg pardon, but which part?  This may be resolved now that I've rejiggered things.

================
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) {
----------------
chandlerc wrote:
> 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.
I added a new traits class, but left the `enable_if` logic in the template for now.  I'll change it if you want, but compare


    template <typename E, bool = IsFlagsEnum<E>::value>
    E &operator|=(E &Lhs, E Rhs) {

to

    template <typename E>
    typename std::enable_if<IsFlagsEnum<E>::value, E &>::type operator|=(E &Lhs,
                                                                         E Rhs) {


http://reviews.llvm.org/D22279