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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

(important bit at the bottom)


================
Comment at: include/llvm/ADT/FlagsEnum.h:24
@@ +23,3 @@
+///
+/// \code
+///   enum MyEnum { E1 = 1, E2 = 2, E3 = 4, E4 = 8};
----------------
At a high level, these things are "bitmask enums" based on the terminology used in the standard (it talks about bitmask types).


After a long, and protracted discussion about the rest of the name, Justin, Richard, and myself realized that the challenge of naming this also pointed at the challenge of communicating *what it does*. The fact that you put this outside the class definition, and between the enum and the macro, the behavior of these operators is *different* is really confusing and subtle.

We really wanted a way to do this so that the moment you declare the enum, things work the way you expect. Sadly, this is pretty much impossible to do in general. Of the things we have to sacrifice, the least-bad sacrifice seems to be name lookup perfection. So our idea was to give up perfect ADL-based lookup of using declarations of the operator overloads, and instead are thinking to just define them in the 'llvm' namespace, provide using declarations in other common project namespaces like 'lld' and 'clang' via a macro and the LLVM.h headers in those projects, and then trigger this behavior in a much simpler way.

The idea is then to just declare a special *enumerator* that triggers these operator overloads to work. This has the advantage of forcing it to reside *inside* the enum's definition, working correctly with both enums nested within classes and those not nested within classes, and not requiring any interesting code to derive from a macro expansion (mucking up diagnostics and such).

The result with hypothetical names would look like this when used:

  enum E {
    ...,
    MyLargest = 1 << N,

    MARK_AS_BITMASK_ENUM(MyLargest)
  };

And would be defined something along the lines of:

  #define MARK_AS_BITMASK_ENUM(Largest) \
      LLVM_BITMASK_LARGEST_ENUMERATOR = Largest
  
  #define EXPAND_BITMASK_ENUM_USING_DECLS \
      using ::llvm::operator?; \
      ...

  namespace llvm {
    template <typename E, typename Enabled = void> struct is_bitmask_enum
        : false_type {};
    template <typename E>
    struct is_bitmask_enum<E, std::enable_if<std::is_enum<decltype(
        E::LLVM_BITMASK_LARGEST_ENUMERATOR)>::value>::type> : true_type {};

    ...
  }

================
Comment at: include/llvm/ADT/FlagsEnum.h:59-60
@@ +58,4 @@
+
+// Traits class to determine whether we can find an
+// llvm_FlagsEnum_GetLargestValue overload for type E via ADL.
+template <typename E, typename Enable = void>
----------------
The ADL part might make more sense below where we actually do the specialization as an implementation comment.

As part of splitting that out, I'd write a (super short) doxygen comment for the trait here.

================
Comment at: include/llvm/ADT/FlagsEnum.h:70-71
@@ +69,4 @@
+
+// Get a bitmask with 1s in all places up to the high-order bit of E's largest
+// value.
+template <typename E> typename std::underlying_type<E>::type Mask() {
----------------
Doxygen here as well.

================
Comment at: include/llvm/ADT/FlagsEnum.h:73-74
@@ +72,4 @@
+template <typename E> typename std::underlying_type<E>::type Mask() {
+  // On overflow, NextPowerOf2 returns uint64_t{0}, so subtracting 1 gives us
+  // the mask with all bits set, like we want.
+  return NextPowerOf2(llvm_FlagsEnum_GetLargestValue(E{})) - 1;
----------------
We don't use braced init syntax like this really anywhere in LLVM, I'd use a more common syntax for writing this or just write it more in prose ("returns zero with the type uint64_t, so ...").


http://reviews.llvm.org/D22279