[sync] Add constexpr to EnumSet

components/sync/base/enum_set.h

Index: components/sync/base/enum_set.h
diff --git a/components/sync/base/enum_set.h b/components/sync/base/enum_set.h
index c6e7b77688a3e1102a52c509fcab15a359d64757..b1aa5a0667692c3d9aaaf07dd0b7a3bb61cb7746 100644
--- a/components/sync/base/enum_set.h
+++ b/components/sync/base/enum_set.h
@@ -119,28 +119,53 @@ class EnumSet {
 
   EnumSet() {}
 
-  // Recursively chain constructors. Base case is the empty pack.
+  ~EnumSet() = default;
+
+  // Base case for recursive packing of a list of enum values. The uint64_t
+  // corresponding to an empty list is 0.
+  static constexpr uint64_t bitstring() { return 0ULL; }
+
+  // Recursively pack a list of enum values into a uint64 so that we can
+  // constexpr construct our bitset. This packing is done by left shifting 1 by

[skym, 2017/05/05 23:15:41]

Can you rework/remove the "This packing is done by ...." sentence, you're
explaining what, not why.

[Patrick Noland, 2017/05/08 20:29:19]

Done.

+  // the current enum value and bitwise or-ing with the bitstring representation
+  // of the rest of the values.
   template <class... T>
-  EnumSet(E head, T... tail) : EnumSet(tail...) {
-    Put(head);
+  static constexpr uint64_t bitstring(E head, T... tail) {
+    return 1ULL << (ToIndex(head)) | bitstring(tail...);

[skym, 2017/05/05 23:15:41]

Not really understanding the precedence of operators, this line makes me uneasy.
I'd feel better with some parens, but if you like this more, feel free to
ignore.

[Patrick Noland, 2017/05/08 20:29:19]

Done.

+  }
+
+  template <class... T>
+  constexpr EnumSet(E head, T... tail)
+      : EnumSet(EnumBitSet(bitstring(head, tail...))) {
+    static_assert(kValueCount < 64,

[skym, 2017/05/05 23:15:41]

Is this really the right place for the static assert?

[Patrick Noland, 2017/05/08 20:29:19]

Done.

+                  "Max number of enum values is 64 for constexpr "
+                  "initialization. If your EnumSet is > 64 values, you can "
+                  "fix this by removing the static_asserts and constexpr "
+                  "qualifiers from this file.");

[skym, 2017/05/05 23:15:41]

What impact will this have? Why is this bad?

[Patrick Noland, 2017/05/08 20:29:19]

Done. I explained that it won't affect correctness, but will be a minor
deoptimization.

   }
 
   // Returns an EnumSet with all possible values.
-  static EnumSet All() {
-    EnumBitSet enums;
-    enums.set();
-    return EnumSet(enums);
+  static constexpr EnumSet All() {
+    static_assert(kValueCount < 64,
+                  "Max number of enum values is 64 for constexpr "

[skym, 2017/05/05 23:15:41]

Lets not duplicate quite so much. Maybe have a big comment explaining things,
and the static assert text can be empty/not present?

[Patrick Noland, 2017/05/08 20:29:19]

Done.

+                  "initialization. If your EnumSet is > 64 values, you can "
+                  "fix this by removing the static_asserts and constexpr "
+                  "qualifiers from this file.");
+    return EnumSet(EnumBitSet((1ULL << kValueCount) - 1));
   }
 
   // Returns an EnumSet with all the values from start to end, inclusive.
-  static EnumSet FromRange(E start, E end) {
-    EnumSet set;
-    set.PutRange(start, end);
-    return set;
+  static constexpr EnumSet FromRange(E start, E end) {
+    static_assert(kValueCount < 64,
+                  "Max number of enum values is 64 for constexpr "

[skym, 2017/05/05 23:15:41]

same

[Patrick Noland, 2017/05/08 20:29:19]

Done.

+                  "initialization. If your EnumSet is > 64 values, you can "
+                  "fix this by removing the static_asserts and constexpr "
+                  "qualifiers from this file.");
+    return EnumSet(
+        EnumBitSet(((1ULL << ToIndex(end)) - (1ULL << ToIndex(start))) |

[skym, 2017/05/05 23:15:41]

Omg this is so clever! Love it! What do you think of having another function
that does the 1ULL << ToIndex(e) operation, it seems to be showing up
everywhere.

[Patrick Noland, 2017/05/08 20:29:19]

Done.

+                   (1ULL << ToIndex(end))));
   }
 
-  ~EnumSet() {}
-
   // Copy constructor and assignment welcome.
 
   // Set operations.  Put, Retain, and Remove are basically
@@ -182,8 +207,8 @@ class EnumSet {
   void Clear() { enums_.reset(); }
 
   // Returns true iff the given value is in range and a member of our set.
-  bool Has(E value) const {
-    return InRange(value) && enums_.test(ToIndex(value));
+  constexpr bool Has(E value) const {
+    return InRange(value) && enums_[ToIndex(value)];
   }
 
   // Returns true iff the given set is a subset of our set.
@@ -215,19 +240,15 @@ class EnumSet {
   friend EnumSet Difference<E, MinEnumValue, MaxEnumValue>(EnumSet set1,
                                                            EnumSet set2);
 
-  explicit EnumSet(EnumBitSet enums) : enums_(enums) {}
+  explicit constexpr EnumSet(EnumBitSet enums) : enums_(enums) {}
 
-  static bool InRange(E value) {
+  static constexpr bool InRange(E value) {
     return (value >= MinEnumValue) && (value <= MaxEnumValue);
   }
 
   // Converts a value to/from an index into |enums_|.
 
-  static size_t ToIndex(E value) {
-    DCHECK_GE(value, MinEnumValue);
-    DCHECK_LE(value, MaxEnumValue);
-    return value - MinEnumValue;
-  }
+  static constexpr size_t ToIndex(E value) { return value - MinEnumValue; }
 
   static E FromIndex(size_t i) {
     DCHECK_LT(i, kValueCount);