[PATCH] D17325: Introduce llvm/ADT/OptionSet.h utility class

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/minor comments.


================
Comment at: include/llvm/ADT/OptionSet.h:51
@@ +50,3 @@
+
+  /// Create an empty option set.
+  OptionSet(llvm::NoneType) : Storage() { }
----------------
How does this differ from the one above it?

================
Comment at: include/llvm/ADT/OptionSet.h:55
@@ +54,3 @@
+  /// Create an option set with only the given option set.
+  OptionSet(Flags flag) : Storage(static_cast<StorageType>(flag)) { }
+
----------------
Please add an assertion that Storage is a power of two inside this constructor.  This is the only place we get to validate that Flag is actually a disjoint bitset.

================
Comment at: include/llvm/ADT/OptionSet.h:74
@@ +73,3 @@
+      std::intptr_t>::type () const {
+    return static_cast<intptr_t>(Storage);
+  }
----------------
Should this be a static_cast<T>?  Otherwise, why have the type param?

================
Comment at: include/llvm/ADT/OptionSet.h:99
@@ +98,3 @@
+  friend OptionSet operator&(OptionSet lhs, OptionSet rhs) {
+    return OptionSet(lhs.Storage & rhs.Storage);
+  }
----------------
minor: expression & in terms of &= or vice versa would make it more clear that both are correct (or equally wrong), but increasing test coverage.  Its very, very likely that any decent compiler will elide the extra copies.  

================
Comment at: unittests/ADT/OptionSetTest.cpp:89
@@ +88,3 @@
+#if !defined(_MSC_VER) || LLVM_MSC_PREREQ(1900)
+// This fails on MSVC 2013; is_constructible returns true despite
+// sizeof(intptr_t) == 4 and sizeof(long long) == 8.
----------------
Seems like this could be handled just like the early constructable return.


http://reviews.llvm.org/D17325