[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:22
@@ +21,3 @@
+/// \brief ENABLE_FLAGS_ENUM lets you opt in an individual enum type so you can
+/// perform bitwise operations on it without putting static_cast everywhere.
+///
----------------
My concern is that this is useful when the enum does not describe flags per-se but where we would like the bit set operations to Just Work. We have a lot of these in LLVM, typically due to lattices in analyses. I feel like flags will be confusing in that context.

I hear your concern that it is a fixed set of bits. Suggestions on a more generic name?

================
Comment at: include/llvm/ADT/FlagsEnum.h:64
@@ +63,3 @@
+template <typename E>
+struct IsFlagsEnum<
+    E, typename std::enable_if<sizeof(llvm_FlagsEnum_GetLargestValue(
----------------
I would name this in the STL's style (we do that a lot but on a completely ad-hoc basis in ADT when something "looks like" an STL-style thing. In this case, this looks like a type trait so I'd use the type trait style.

================
Comment at: include/llvm/ADT/FlagsEnum.h:69
@@ +68,3 @@
+
+// Get a bitmask with 1s in all places up to the high-order bit of E's largest
+// value.
----------------
Your suggested syntax is actually my personal preference to write, however I remembered that Clang has special, very nice diagnostics for enable_if patterns that won't fire with it and will fire with the second version. =/

I guess I would suggest:

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

It's a lot more typing, but Richard Smith and I are conspiring on a way to make this much more brief.

I would request 'RHS' and 'LHS' instead of 'Rhs' and 'Lhs' here and elsewhere.


http://reviews.llvm.org/D22279