diff mbox

Implement P0393R3

Message ID CAG4ZjNm8UCMAKrHwoA7KcdmQkETY26kc8LE9YsWpSTti5Nhvng@mail.gmail.com
State New
Headers show

Commit Message

Li, Pan2 via Gcc-patches Jan. 9, 2017, 6:49 a.m. UTC 
On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 01/01/17 04:17 -0800, Tim Shen via libstdc++ wrote:
>>
>> +#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
>> +  template<typename... _Types> \
>> +    constexpr bool operator __op(const variant<_Types...>& __lhs, \
>> +                                const variant<_Types...>& __rhs) \
>> +    { \
>> +      return __lhs._M##__name(__rhs,
>> std::index_sequence_for<_Types...>{}); \
>> +    } \
>> +\
>> +  constexpr bool operator __op(monostate, monostate) noexcept \
>> +  { return 0 __op 0; }
>> +
>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, _erased_less_than)
>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, _erased_less_equal)
>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, _erased_equal)
>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, _erased_not_equal)
>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, _erased_greater_than)
>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, _erased_greater)
>
>
> These need double underscore prefixes.

Done.

>
> Still reviewing the rest ...
>

Comments

Jonathan Wakely Jan. 9, 2017, 10:52 a.m. UTC | #1 
On 08/01/17 22:49 -0800, Tim Shen wrote:
>On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 01/01/17 04:17 -0800, Tim Shen via libstdc++ wrote:
>>>
>>> +#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
>>> +  template<typename... _Types> \
>>> +    constexpr bool operator __op(const variant<_Types...>& __lhs, \
>>> +                                const variant<_Types...>& __rhs) \
>>> +    { \
>>> +      return __lhs._M##__name(__rhs,
>>> std::index_sequence_for<_Types...>{}); \
>>> +    } \
>>> +\
>>> +  constexpr bool operator __op(monostate, monostate) noexcept \
>>> +  { return 0 __op 0; }
>>> +
>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, _erased_less_than)
>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, _erased_less_equal)
>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, _erased_equal)
>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, _erased_not_equal)
>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, _erased_greater_than)
>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, _erased_greater)
>>
>>
>> These need double underscore prefixes.
>
>Done.

I'm sorry, I missed that they get appended to _M to form a member
function name, so they don't need a double underscore.

But since they all have the same prefix, why not use _M_erased_##name
and just use less_than, less_equal etc. in the macro invocations?

However, the names are weird, you have >= as greater_than (not
greater_equal) and > as greater (which is inconsistent with < as
less_than).

So I'd go with:

_VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
_VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
_VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
_VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
_VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
_VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)

>+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \

I think we usually use all-caps for macro arguments, so _OP and _NAME,
but it doesn't really matter.

>+      template<size_t... __indices> \
>+	static constexpr bool \
>+	(*_S##__name##_vtable[])(const variant&, const variant&) = \
>+	  { &__detail::__variant::__name<const variant&, __indices>... }; \

With the suggestions above this would change to use _S_erased_##_NAME
and &__detail::__variant::__erased_##_NAME

>+      template<size_t... __indices> \
>+	constexpr inline bool \
>+	_M##__name(const variant& __rhs, \
>+		     std::index_sequence<__indices...>) const \
>+	{ \
>+	  auto __lhs_index = this->index(); \
>+	  auto __rhs_index = __rhs.index(); \
>+	  if (__lhs_index != __rhs_index || valueless_by_exception()) \
>+	    /* Intentinoal modulo addition. */ \

"Intentional" is spelled wrong, but I think simply "Modulo addition"
is clear enough that it's intentional.

>+	    return __lhs_index + 1 __op __rhs_index + 1; \
>+	  return _S##__name##_vtable<__indices...>[__lhs_index](*this, __rhs); \
> 	}
>
>-      template<size_t... __indices>

And we'd usually use _Indices for template parameters, but this is
already inconsistent in <variant>.

The patch is OK with those naming tweaks. Thanks, and sorry for the
mixup about the underscores.
Li, Pan2 via Gcc-patches Feb. 15, 2017, 9:06 a.m. UTC | #2 
On Mon, Jan 9, 2017 at 2:52 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 08/01/17 22:49 -0800, Tim Shen wrote:
>>
>> On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely <jwakely@redhat.com>
>> wrote:
>>>
>>> On 01/01/17 04:17 -0800, Tim Shen via libstdc++ wrote:
>>>>
>>>>
>>>> +#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
>>>> +  template<typename... _Types> \
>>>> +    constexpr bool operator __op(const variant<_Types...>& __lhs, \
>>>> +                                const variant<_Types...>& __rhs) \
>>>> +    { \
>>>> +      return __lhs._M##__name(__rhs,
>>>> std::index_sequence_for<_Types...>{}); \
>>>> +    } \
>>>> +\
>>>> +  constexpr bool operator __op(monostate, monostate) noexcept \
>>>> +  { return 0 __op 0; }
>>>> +
>>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, _erased_less_than)
>>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, _erased_less_equal)
>>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, _erased_equal)
>>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, _erased_not_equal)
>>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, _erased_greater_than)
>>>> +  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, _erased_greater)
>>>
>>>
>>>
>>> These need double underscore prefixes.
>>
>>
>> Done.
>
>
> I'm sorry, I missed that they get appended to _M to form a member
> function name, so they don't need a double underscore.
>
> But since they all have the same prefix, why not use _M_erased_##name
> and just use less_than, less_equal etc. in the macro invocations?
>
> However, the names are weird, you have >= as greater_than (not
> greater_equal) and > as greater (which is inconsistent with < as
> less_than).
>
> So I'd go with:
>
> _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
> _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
> _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
> _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
> _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
> _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
>
>> +#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
>
>
> I think we usually use all-caps for macro arguments, so _OP and _NAME,
> but it doesn't really matter.
>
>> +      template<size_t... __indices> \
>> +       static constexpr bool \
>> +       (*_S##__name##_vtable[])(const variant&, const variant&) = \
>> +         { &__detail::__variant::__name<const variant&, __indices>... };
>> \
>
>
> With the suggestions above this would change to use _S_erased_##_NAME
> and &__detail::__variant::__erased_##_NAME
>
>> +      template<size_t... __indices> \
>> +       constexpr inline bool \
>> +       _M##__name(const variant& __rhs, \
>> +                    std::index_sequence<__indices...>) const \
>> +       { \
>> +         auto __lhs_index = this->index(); \
>> +         auto __rhs_index = __rhs.index(); \
>> +         if (__lhs_index != __rhs_index || valueless_by_exception()) \
>> +           /* Intentinoal modulo addition. */ \
>
>
> "Intentional" is spelled wrong, but I think simply "Modulo addition"
> is clear enough that it's intentional.
>
>> +           return __lhs_index + 1 __op __rhs_index + 1; \
>> +         return _S##__name##_vtable<__indices...>[__lhs_index](*this,
>> __rhs); \
>>         }

All done.

>>
>> -      template<size_t... __indices>
>
>
> And we'd usually use _Indices for template parameters, but this is
> already inconsistent in <variant>.

I didn't change this one, since I prefer in-file consistency. That's
weird, So let's discuss this. There are several naming style in
libstdc++:
1) __underscore_name, for free functions, variables, and namespaces;
2) _CamelCase, for types, template type parameters in some files (e.g.
<regex>, I forgot why I did that :/).
3) _Camel_underscore_name, for types in many other files.
4) _S_underscore_name, _M_underscore name, for static and member
functions, respectively.

There are two questions:
*) It seems natural to have (1) for non-type template parameters,
since that are also variables.
*) _CamelCase vs _Camel_underscore_name? Which one is preferred?

>
> The patch is OK with those naming tweaks. Thanks, and sorry for the
> mixup about the underscores.
>

No worries! Tested and committed.
diff mbox

Patch

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 3d025a7..9ca61d6 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -263,21 +263,23 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       swap(__ref_cast<_Lhs>(__lhs), __ref_cast<_Rhs>(__rhs));
     }
 
-  template<typename _Variant, size_t _Np>
-    constexpr bool
-    __erased_equal_to(_Variant&& __lhs, _Variant&& __rhs)
-    {
-      return __get<_Np>(std::forward<_Variant>(__lhs))
-	  == __get<_Np>(std::forward<_Variant>(__rhs));
+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __function_name) \
+  template<typename _Variant, size_t _Np> \
+    constexpr bool \
+    __function_name(const _Variant& __lhs, const _Variant& __rhs) \
+    { \
+      return __get<_Np>(std::forward<_Variant>(__lhs)) \
+	  __op __get<_Np>(std::forward<_Variant>(__rhs)); \
     }
 
-  template<typename _Variant, size_t _Np>
-    constexpr bool
-    __erased_less_than(const _Variant& __lhs, const _Variant& __rhs)
-    {
-      return __get<_Np>(std::forward<_Variant>(__lhs))
-	  < __get<_Np>(std::forward<_Variant>(__rhs));
-    }
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, __erased_less_than)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, __erased_less_equal)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, __erased_equal)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, __erased_not_equal)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, __erased_greater_than)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, __erased_greater)
+
+#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
 
   template<typename _Tp>
     constexpr size_t
@@ -800,63 +802,31 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return get_if<__detail::__variant::__index_of_v<_Tp, _Types...>>(__ptr);
     }
 
-  template<typename... _Types>
-    constexpr bool operator==(const variant<_Types...>& __lhs,
-			      const variant<_Types...>& __rhs)
-    {
-      return __lhs._M_equal_to(__rhs, std::index_sequence_for<_Types...>{});
-    }
-
-  template<typename... _Types>
-    constexpr inline bool
-    operator!=(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-    { return !(__lhs == __rhs); }
-
-  template<typename... _Types>
-    constexpr inline bool
-    operator<(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-    {
-      return __lhs._M_less_than(__rhs, std::index_sequence_for<_Types...>{});
-    }
-
-  template<typename... _Types>
-    constexpr inline bool
-    operator>(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-    { return __rhs < __lhs; }
-
-  template<typename... _Types>
-    constexpr inline bool
-    operator<=(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-    { return !(__lhs > __rhs); }
+  struct monostate { };
 
-  template<typename... _Types>
-    constexpr inline bool
-    operator>=(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-    { return !(__lhs < __rhs); }
+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
+  template<typename... _Types> \
+    constexpr bool operator __op(const variant<_Types...>& __lhs, \
+				 const variant<_Types...>& __rhs) \
+    { \
+      return __lhs._M##__name(__rhs, std::index_sequence_for<_Types...>{}); \
+    } \
+\
+  constexpr bool operator __op(monostate, monostate) noexcept \
+  { return 0 __op 0; }
+
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, __erased_less_than)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, __erased_less_equal)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, __erased_equal)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, __erased_not_equal)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, __erased_greater_than)
+  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, __erased_greater)
+
+#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
 
   template<typename _Visitor, typename... _Variants>
     constexpr decltype(auto) visit(_Visitor&&, _Variants&&...);
 
-  struct monostate { };
-
-  constexpr bool operator<(monostate, monostate) noexcept
-  { return false; }
-
-  constexpr bool operator>(monostate, monostate) noexcept
-  { return false; }
-
-  constexpr bool operator<=(monostate, monostate) noexcept
-  { return true; }
-
-  constexpr bool operator>=(monostate, monostate) noexcept
-  { return true; }
-
-  constexpr bool operator==(monostate, monostate) noexcept
-  { return true; }
-
-  constexpr bool operator!=(monostate, monostate) noexcept
-  { return false; }
-
   template<typename... _Types>
     inline enable_if_t<(is_move_constructible_v<_Types> && ...)
 			&& (is_swappable_v<_Types> && ...)>
@@ -1122,51 +1092,32 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
 
     private:
-      template<size_t... __indices>
-	static constexpr bool
-	(*_S_equal_to_vtable[])(const variant&, const variant&) =
-	  { &__detail::__variant::__erased_equal_to<
-	    const variant&, __indices>... };
-
-      template<size_t... __indices>
-	static constexpr bool
-	(*_S_less_than_vtable[])(const variant&, const variant&) =
-	  { &__detail::__variant::__erased_less_than<
-	      const variant&, __indices>... };
-
-      template<size_t... __indices>
-	constexpr bool
-	_M_equal_to(const variant& __rhs,
-		    std::index_sequence<__indices...>) const
-	{
-	  if (this->index() != __rhs.index())
-	    return false;
-
-	  if (this->valueless_by_exception())
-	    return true;
-
-	  return _S_equal_to_vtable<__indices...>[this->index()](*this, __rhs);
+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
+      template<size_t... __indices> \
+	static constexpr bool \
+	(*_S##__name##_vtable[])(const variant&, const variant&) = \
+	  { &__detail::__variant::__name<const variant&, __indices>... }; \
+      template<size_t... __indices> \
+	constexpr inline bool \
+	_M##__name(const variant& __rhs, \
+		     std::index_sequence<__indices...>) const \
+	{ \
+	  auto __lhs_index = this->index(); \
+	  auto __rhs_index = __rhs.index(); \
+	  if (__lhs_index != __rhs_index || valueless_by_exception()) \
+	    /* Intentinoal modulo addition. */ \
+	    return __lhs_index + 1 __op __rhs_index + 1; \
+	  return _S##__name##_vtable<__indices...>[__lhs_index](*this, __rhs); \
 	}
 
-      template<size_t... __indices>
-	constexpr inline bool
-	_M_less_than(const variant& __rhs,
-		     std::index_sequence<__indices...>) const
-	{
-	  auto __lhs_index = this->index();
-	  auto __rhs_index = __rhs.index();
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(<, __erased_less_than)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, __erased_less_equal)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(==, __erased_equal)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, __erased_not_equal)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, __erased_greater_than)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(>, __erased_greater)
 
-	  if (__lhs_index < __rhs_index)
-	    return true;
-
-	  if (__lhs_index > __rhs_index)
-	    return false;
-
-	  if (this->valueless_by_exception())
-	    return false;
-
-	  return _S_less_than_vtable<__indices...>[__lhs_index](*this, __rhs);
-	}
+#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
 
       template<size_t _Np, typename _Vp>
 	friend constexpr decltype(auto) __detail::__variant::
@@ -1182,15 +1133,20 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #endif
         __get_storage(_Vp&& __v);
 
-      template<typename... _Tp>
-	friend constexpr bool
-	operator==(const variant<_Tp...>& __lhs,
-		   const variant<_Tp...>& __rhs);
+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op) \
+      template<typename... _Tp> \
+	friend constexpr bool \
+	operator __op(const variant<_Tp...>& __lhs, \
+		      const variant<_Tp...>& __rhs);
+
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(<)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(<=)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(==)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(!=)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(>=)
+      _VARIANT_RELATION_FUNCTION_TEMPLATE(>)
 
-      template<typename... _Tp>
-	friend constexpr bool
-	operator<(const variant<_Tp...>& __lhs,
-		  const variant<_Tp...>& __rhs);
+#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
     };
 
   template<size_t _Np, typename... _Types>
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index 65f4326..8320013 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -65,7 +65,11 @@  struct nonliteral
   nonliteral() { }
 
   bool operator<(const nonliteral&) const;
+  bool operator<=(const nonliteral&) const;
   bool operator==(const nonliteral&) const;
+  bool operator!=(const nonliteral&) const;
+  bool operator>=(const nonliteral&) const;
+  bool operator>(const nonliteral&) const;
 };
 
 void default_ctor()
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index 121fd22..db4529e 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -47,6 +47,13 @@  struct AlwaysThrow
     throw nullptr;
     return *this;
   }
+
+  bool operator<(const AlwaysThrow&) const { VERIFY(false); }
+  bool operator<=(const AlwaysThrow&) const { VERIFY(false); }
+  bool operator==(const AlwaysThrow&) const { VERIFY(false); }
+  bool operator!=(const AlwaysThrow&) const { VERIFY(false); }
+  bool operator>=(const AlwaysThrow&) const { VERIFY(false); }
+  bool operator>(const AlwaysThrow&) const { VERIFY(false); }
 };
 
 void default_ctor()
@@ -229,6 +236,23 @@  void test_relational()
 
   VERIFY((variant<int, string>(2) < variant<int, string>("a")));
   VERIFY((variant<string, int>(2) > variant<string, int>("a")));
+
+  {
+    variant<int, AlwaysThrow> v, w;
+    try
+      {
+	AlwaysThrow a;
+	v = a;
+      }
+    catch (nullptr_t) { }
+    VERIFY(v.valueless_by_exception());
+    VERIFY(v < w);
+    VERIFY(v <= w);
+    VERIFY(!(v == w));
+    VERIFY(v != w);
+    VERIFY(w > v);
+    VERIFY(w >= v);
+  }
 }
 
 void test_swap()