Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them - r=froydnj draft
authorGerald Squelart <gsquelart@mozilla.com>
Wed, 10 May 2017 15:49:38 +1200
changeset 589408 fed56f264821e87cc474e6686e6dc203c147ac9b
parent 589407 d88cf614318cc8544d7ab52315e015a7ca4e5efd
child 589409 d59375a7c805d6d470f5074669f3db5a609eb518
push id62356
push usergsquelart@mozilla.com
push dateTue, 06 Jun 2017 05:57:26 +0000
reviewersfroydnj
bugs1338389
milestone55.0a1
Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them - r=froydnj MozReview-Commit-ID: 1yEUuGsht8k
mfbt/Variant.h
mfbt/tests/TestVariant.cpp
--- a/mfbt/Variant.h
+++ b/mfbt/Variant.h
@@ -36,102 +36,66 @@ struct Nth<0, T, Ts...>
 };
 
 template<size_t N, typename T, typename... Ts>
 struct Nth<N, T, Ts...>
 {
   using Type = typename Nth<N - 1, Ts...>::Type;
 };
 
-template <typename...>
-struct FirstTypeIsInRest;
-
-template <typename First>
-struct FirstTypeIsInRest<First> : FalseType {};
-
-template <typename First, typename Second, typename... Rest>
-struct FirstTypeIsInRest<First, Second, Rest...>
-{
-  static constexpr bool value =
-    IsSame<First, Second>::value ||
-    FirstTypeIsInRest<First, Rest...>::value;
-};
-
-template <typename...>
-struct TypesAreDistinct;
-
-template <>
-struct TypesAreDistinct<> : TrueType { };
-
-template<typename First, typename... Rest>
-struct TypesAreDistinct<First, Rest...>
-{
-  static constexpr bool value =
-    !FirstTypeIsInRest<First, Rest...>::value &&
-    TypesAreDistinct<Rest...>::value;
-};
-
-// The `IsVariant` helper is used in conjunction with static_assert and
-// `mozilla::EnableIf` to catch passing non-variant types to `Variant::is<T>()`
-// and friends at compile time, rather than at runtime. It ensures that the
-// given type `Needle` is one of the types in the set of types `Haystack`.
-
-template<typename Needle, typename... Haystack>
-struct IsVariant;
-
-template<typename Needle>
-struct IsVariant<Needle> : FalseType {};
-
-template<typename Needle, typename... Haystack>
-struct IsVariant<Needle, Needle, Haystack...> : TrueType {};
-
-template<typename Needle, typename T, typename... Haystack>
-struct IsVariant<Needle, T, Haystack...> : public IsVariant<Needle, Haystack...> { };
-
 /// SelectVariantTypeHelper is used in the implementation of SelectVariantType.
 template<typename T, typename... Variants>
 struct SelectVariantTypeHelper;
 
 template<typename T>
 struct SelectVariantTypeHelper<T>
-{ };
+{
+  static constexpr size_t count = 0;
+};
 
 template<typename T, typename... Variants>
 struct SelectVariantTypeHelper<T, T, Variants...>
 {
   typedef T Type;
+  static constexpr size_t count = 1 + SelectVariantTypeHelper<T, Variants...>::count;
 };
 
 template<typename T, typename... Variants>
 struct SelectVariantTypeHelper<T, const T, Variants...>
 {
   typedef const T Type;
+  static constexpr size_t count = 1 + SelectVariantTypeHelper<T, Variants...>::count;
 };
 
 template<typename T, typename... Variants>
 struct SelectVariantTypeHelper<T, const T&, Variants...>
 {
   typedef const T& Type;
+  static constexpr size_t count = 1 + SelectVariantTypeHelper<T, Variants...>::count;
 };
 
 template<typename T, typename... Variants>
 struct SelectVariantTypeHelper<T, T&&, Variants...>
 {
   typedef T&& Type;
+  static constexpr size_t count = 1 + SelectVariantTypeHelper<T, Variants...>::count;
 };
 
 template<typename T, typename Head, typename... Variants>
 struct SelectVariantTypeHelper<T, Head, Variants...>
   : public SelectVariantTypeHelper<T, Variants...>
 { };
 
 /**
  * SelectVariantType takes a type T and a list of variant types Variants and
  * yields a type Type, selected from Variants, that can store a value of type T
  * or a reference to type T. If no such type was found, Type is not defined.
+ * SelectVariantType also has a `count` member that contains the total number of
+ * selectable types (which will be used to check that a requested type is not
+ * ambiguously present twice.)
  */
 template <typename T, typename... Variants>
 struct SelectVariantType
   : public SelectVariantTypeHelper<typename RemoveConst<typename RemoveReference<T>::Type>::Type,
                                    Variants...>
 { };
 
 // Compute a fast, compact type that can be used to hold integral values that
@@ -368,23 +332,53 @@ struct AsVariantTemporary
  * Either the stored type, or the type index may be provided.
  *
  *     void
  *     Foo(Variant<A, B, C> v)
  *     {
  *       if (v.is<A>()) {
  *         A& ref = v.as<A>();
  *         ...
- *       } else (v.is<1>()) { // Same as v.is<B> in this case.
+ *       } else (v.is<1>()) { // Instead of v.is<B>.
  *         ...
  *       } else {
  *         ...
  *       }
  *     }
  *
+ * In some situation, a Variant may be constructed from templated types, in
+ * which case it is possible that the same type could be given multiple times by
+ * an external developer. Or seemingly-different types could be aliases.
+ * In this case, repeated types can only be accessed through their index, to
+ * prevent ambiguous access by type.
+ *
+ *    // Bad!
+ *    template <typename T>
+ *    struct ResultOrError
+ *    {
+ *      Variant<T, int> m;
+ *      bool IsResult() const { return m.is<T>(); }
+ *      bool IsError() const { return m.is<int>(); }
+ *    };
+ *    // Now instantiante with the result being an int too:
+ *    ResultOrError<int> myResult; // Fail!
+ *    // In Variant<int, int>, which 'int' are we refering to, from inside
+ *    // ResultOrError functions?
+ *
+ *    // Good!
+ *    template <typename T>
+ *    struct ResultOrError
+ *    {
+ *      Variant<T, int> m;
+ *      bool IsResult() const { return m.is<0>(); } // 0 -> T
+ *      bool IsError() const { return m.is<1>(); } // 1 -> int
+ *    };
+ *    // Now instantiante with the result being an int too:
+ *    ResultOrError<int> myResult; // It now works!
+ *
  * Attempting to use the contained value as type `T1` when the `Variant`
  * instance contains a value of type `T2` causes an assertion failure.
  *
  *     A a;
  *     Variant<A, B, C> v(a);
  *     v.as<B>(); // <--- Assertion failure!
  *
  * Trying to use a `Variant<Ts...>` instance as some type `U` that is not a
@@ -461,19 +455,16 @@ struct AsVariantTemporary
  * and because |alignas| requirements don't affect platform ABI with respect to
  * how parameters are laid out in memory, Variant can't be used as the type of a
  * function parameter.  Pass Variant to functions by pointer or reference
  * instead.
  */
 template<typename... Ts>
 class MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS MOZ_NON_PARAM Variant
 {
-  static_assert(detail::TypesAreDistinct<Ts...>::value,
-                "Variant with duplicate types is not supported");
-
   using Tag = typename detail::VariantTag<Ts...>::Type;
   using Impl = detail::VariantImplementation<Tag, 0, Ts...>;
 
   static constexpr size_t RawDataAlignment = tl::Max<alignof(Ts)...>::value;
   static constexpr size_t RawDataSize = tl::Max<sizeof(Ts)...>::value;
 
   // Raw storage for the contained variant value.
   alignas(RawDataAlignment) unsigned char rawData[RawDataSize];
@@ -500,29 +491,33 @@ public:
            // RefT captures both const& as well as && (as intended, to support
            // perfect forwarding), so we have to remove those qualifiers here
            // when ensuring that T is a variant of this type, and getting T's
            // tag, etc.
            typename T = typename detail::SelectVariantType<RefT, Ts...>::Type>
   explicit Variant(RefT&& aT)
     : tag(Impl::template tag<T>())
   {
+    static_assert(detail::SelectVariantType<RefT, Ts...>::count == 1,
+                  "Variant can only be selected by type if that type is unique");
     ::new (KnownNotNull, ptr()) T(Forward<RefT>(aT));
   }
 
   /**
    * Constructs this Variant from an AsVariantTemporary<T> such that T can be
    * stored in one of the types allowable in this Variant. This is used in the
    * implementation of AsVariant().
    */
   template<typename RefT,
            typename T = typename detail::SelectVariantType<RefT, Ts...>::Type>
   MOZ_IMPLICIT Variant(detail::AsVariantTemporary<RefT>&& aValue)
     : tag(Impl::template tag<T>())
   {
+    static_assert(detail::SelectVariantType<RefT, Ts...>::count == 1,
+                  "Variant can only be selected by type if that type is unique");
     ::new (KnownNotNull, ptr()) T(Move(aValue.mValue));
   }
 
   /** Copy construction. */
   Variant(const Variant& aRhs)
     : tag(aRhs.tag)
   {
     Impl::copyConstruct(ptr(), aRhs);
@@ -550,31 +545,33 @@ public:
     ::new (KnownNotNull, this) Variant(Move(aRhs));
     return *this;
   }
 
   /** Move assignment from AsVariant(). */
   template<typename T>
   Variant& operator=(detail::AsVariantTemporary<T>&& aValue)
   {
+    static_assert(detail::SelectVariantType<T, Ts...>::count == 1,
+                  "Variant can only be selected by type if that type is unique");
     this->~Variant();
     ::new (KnownNotNull, this) Variant(Move(aValue));
     return *this;
   }
 
   ~Variant()
   {
     Impl::destroy(*this);
   }
 
   /** Check which variant type is currently contained. */
   template<typename T>
   bool is() const {
-    static_assert(detail::IsVariant<T, Ts...>::value,
-                  "provided a type not found in this Variant's type list");
+    static_assert(detail::SelectVariantType<T, Ts...>::count == 1,
+                  "provided a type not uniquely found in this Variant's type list");
     return Impl::template tag<T>() == tag;
   }
 
   template<size_t N>
   bool is() const
   {
     static_assert(N < sizeof...(Ts),
                   "provided an index outside of this Variant's type list");
@@ -598,35 +595,35 @@ public:
     return !(*this == aRhs);
   }
 
   // Accessors for working with the contained variant value.
 
   /** Mutable reference. */
   template<typename T>
   T& as() {
-    static_assert(detail::IsVariant<T, Ts...>::value,
-                  "provided a type not found in this Variant's type list");
+    static_assert(detail::SelectVariantType<T, Ts...>::count == 1,
+                  "provided a type not uniquely found in this Variant's type list");
     MOZ_RELEASE_ASSERT(is<T>());
     return *static_cast<T*>(ptr());
   }
 
   template<size_t N>
   typename detail::Nth<N, Ts...>::Type& as()
   {
     static_assert(N < sizeof...(Ts),
                   "provided an index outside of this Variant's type list");
     MOZ_RELEASE_ASSERT(is<N>());
     return *static_cast<typename detail::Nth<N, Ts...>::Type*>(ptr());
   }
 
   /** Immutable const reference. */
   template<typename T>
   const T& as() const {
-    static_assert(detail::IsVariant<T, Ts...>::value,
+    static_assert(detail::SelectVariantType<T, Ts...>::count == 1,
                   "provided a type not found in this Variant's type list");
     MOZ_RELEASE_ASSERT(is<T>());
     return *static_cast<const T*>(ptr());
   }
 
   template<size_t N>
   const typename detail::Nth<N, Ts...>::Type& as() const
   {
@@ -639,18 +636,18 @@ public:
   /**
    * Extract the contained variant value from this container into a temporary
    * value.  On completion, the value in the variant will be in a
    * safely-destructible state, as determined by the behavior of T's move
    * constructor when provided the variant's internal value.
    */
   template<typename T>
   T extract() {
-    static_assert(detail::IsVariant<T, Ts...>::value,
-                  "provided a type not found in this Variant's type list");
+    static_assert(detail::SelectVariantType<T, Ts...>::count == 1,
+                  "provided a type not uniquely found in this Variant's type list");
     MOZ_ASSERT(is<T>());
     return T(Move(as<T>()));
   }
 
   template<size_t N>
   typename detail::Nth<N, Ts...>::Type extract()
   {
     static_assert(N < sizeof...(Ts),
--- a/mfbt/tests/TestVariant.cpp
+++ b/mfbt/tests/TestVariant.cpp
@@ -32,16 +32,38 @@ testSimple()
   MOZ_RELEASE_ASSERT(v.is<1>());
   MOZ_RELEASE_ASSERT(!v.is<0>());
   static_assert(mozilla::IsSame<decltype(v.as<1>()), uint64_t&>::value,
                 "as<1>() should return a uint64_t");
   MOZ_RELEASE_ASSERT(v.as<1>() == 1);
 }
 
 static void
+testDuplicate()
+{
+  printf("testDuplicate\n");
+  Variant<uint32_t, uint64_t, uint32_t> v(uint64_t(1));
+  MOZ_RELEASE_ASSERT(v.is<uint64_t>());
+  MOZ_RELEASE_ASSERT(v.as<uint64_t>() == 1);
+  // Note: uint32_t is not unique, so `v.is<uint32_t>()` is not allowed.
+
+  MOZ_RELEASE_ASSERT(v.is<1>());
+  MOZ_RELEASE_ASSERT(!v.is<0>());
+  MOZ_RELEASE_ASSERT(!v.is<2>());
+  static_assert(mozilla::IsSame<decltype(v.as<0>()), uint32_t&>::value,
+                "as<0>() should return a uint64_t");
+  static_assert(mozilla::IsSame<decltype(v.as<1>()), uint64_t&>::value,
+                "as<1>() should return a uint64_t");
+  static_assert(mozilla::IsSame<decltype(v.as<2>()), uint32_t&>::value,
+                "as<2>() should return a uint64_t");
+  MOZ_RELEASE_ASSERT(v.as<1>() == 1);
+  MOZ_RELEASE_ASSERT(v.extract<1>() == 1);
+}
+
+static void
 testCopy()
 {
   printf("testCopy\n");
   Variant<uint32_t, uint64_t> v1(uint64_t(1));
   Variant<uint32_t, uint64_t> v2(v1);
   MOZ_RELEASE_ASSERT(v2.is<uint64_t>());
   MOZ_RELEASE_ASSERT(!v2.is<uint32_t>());
   MOZ_RELEASE_ASSERT(v2.as<uint64_t>() == 1);
@@ -177,16 +199,17 @@ testRvalueMatcher()
   V v(uint8_t(1));
   MOZ_RELEASE_ASSERT(v.match(Describer()) == Describer::little);
 }
 
 int
 main()
 {
   testSimple();
+  testDuplicate();
   testCopy();
   testMove();
   testDestructor();
   testEquality();
   testMatching();
   testRvalueMatcher();
 
   printf("TestVariant OK!\n");