[sync] Add constexpr to EnumSet

components/sync/base/enum_set.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_SYNC_BASE_ENUM_SET_H_
 #define COMPONENTS_SYNC_BASE_ENUM_SET_H_
 
 #include <bitset>
 #include <cstddef>
 #include <string>
@@ skipping 101 matching lines @@
       }
       return i;
     }
 
     const EnumBitSet* enums_;
     size_t i_;
   };
 
   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
   // self-mutating versions of Union, Intersection, and Difference
   // (defined below).
 
   // Adds the given value (which must be in range) to our set.
   void Put(E value) { enums_.set(ToIndex(value)); }
 
   // Adds all values in the given set to our set.
@@ skipping 21 matching lines @@
     }
   }
 
   // Removes all values in the given set from our set.
   void RemoveAll(EnumSet other) { enums_ &= ~other.enums_; }
 
   // Removes all values from our set.
   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.
   bool HasAll(EnumSet other) const {
     return (enums_ & other.enums_) == other.enums_;
   }
 
   // Returns true iff our set is empty.
   bool Empty() const { return !enums_.any(); }
 
@@ skipping 11 matching lines @@
   bool operator!=(const EnumSet& other) const { return enums_ != other.enums_; }
 
  private:
   friend EnumSet Union<E, MinEnumValue, MaxEnumValue>(EnumSet set1,
                                                       EnumSet set2);
   friend EnumSet Intersection<E, MinEnumValue, MaxEnumValue>(EnumSet set1,
                                                              EnumSet set2);
   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);
     return static_cast<E>(MinEnumValue + i);
   }
 
   EnumBitSet enums_;
 };
 
 template <typename E, E MinEnumValue, E MaxEnumValue>
@@ skipping 21 matching lines @@
 
 template <typename E, E Min, E Max>
 EnumSet<E, Min, Max> Difference(EnumSet<E, Min, Max> set1,
                                 EnumSet<E, Min, Max> set2) {
   return EnumSet<E, Min, Max>(set1.enums_ & ~set2.enums_);
 }
 
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_BASE_ENUM_SET_H_