Re: [PATCH] Implement P0393R3

2017-02-15 Thread Tim Shen via gcc-patches 
On Mon, Jan 9, 2017 at 2:52 AM, Jonathan Wakely  wrote:
> On 08/01/17 22:49 -0800, Tim Shen wrote:
>>
>> On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely 
>> wrote:
>>>
>>> On 01/01/17 04:17 -0800, Tim Shen via libstdc++ wrote:


 +#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
 +  template \
 +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 \
>> +   static constexpr bool \
>> +   (*_S##__name##_vtable[])(const variant&, const variant&) = \
>> + { &__detail::__variant::__name... };
>> \
>
>
> With the suggestions above this would change to use _S_erased_##_NAME
> and &__detail::__variant::__erased_##_NAME
>
>> +  template \
>> +   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
>
>
> And we'd usually use _Indices for template parameters, but this is
> already inconsistent in .

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.
, 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.


-- 
Regards,
Tim Shen

Re: [PATCH] Implement P0393R3

2017-01-09 Thread Jonathan Wakely 

On 08/01/17 22:49 -0800, Tim Shen wrote:

On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely  wrote:

On 01/01/17 04:17 -0800, Tim Shen via libstdc++ wrote:


+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
+  template \
+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 \
+   static constexpr bool \
+   (*_S##__name##_vtable[])(const variant&, const variant&) = \
+ { &__detail::__variant::__name... }; \


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


+  template \
+   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


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

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

Re: [PATCH] Implement P0393R3

2017-01-08 Thread Tim Shen via gcc-patches 
On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely  wrote:
> On 01/01/17 04:17 -0800, Tim Shen via libstdc++ wrote:
>>
>> +#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
>> +  template \
>> +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 ...
>



-- 
Regards,
Tim Shen
commit fba8c3c8cca773a501766aff90b13f72e42d9355
Author: Tim Shen 
Date:   Sun Jan 1 04:07:15 2017 -0800

2017-01-01  Tim Shen  

PR libstdc++/78723
* include/std/variant: Implement P0393R3.
* testsuite/20_util/variant/compile.cc: Adjust tests.
* testsuite/20_util/variant/run.cc: Adjust tests.

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
-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 \
+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
-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
 constexpr size_t
@@ -800,63 +802,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   return get_if<__detail::__variant::__index_of_v<_Tp, _Types...>>(__ptr);
 }
 
-  template
-constexpr bool operator==(const variant<_Types...>& __lhs,
- const variant<_Types...>& __rhs)
-{
-  return __lhs._M_equal_to(__rhs, std::index_sequence_for<_Types...>{});
-}
-
-  template
-constexpr inline bool
-operator!=(const variant<_Types...>& __lhs, const variant<_Types...>& 
__rhs)
-{ return !(__lhs == __rhs); }
-
-  template
-constexpr inline bool
-operator<(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-{
-  return __lhs._M_less_than(__rhs, std::index_sequence_for<_Types...>{});
-}
-
-  template
-constexpr inline bool
-operator>(const variant<_Types...>& __lhs, const variant<_Types...>& __rhs)
-{ return __rhs < __lhs; }
-
-  template
-constexpr inline bool
-operator<=(const variant<_Types...>& __lhs, const variant<_Types...>& 
__rhs)
-{ return !(__lhs > __rhs); }
+  struct monostate { };
 
-  template
-constexpr inline bool
-operator>=(const variant<_Types...>& __lhs, const variant<_Types...>& 
__rhs)
-{ return !(__lhs < __rhs); }
+#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__op, __name) \
+  template \
+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(>,