[PATCH] Implement P0393R3

[Tim Shen via libstdc++]

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.


-- 
Regards,
Tim Shen