On Mon, Jan 9, 2017 at 2:52 AM, Jonathan Wakely <jwak...@redhat.com> wrote:
> On 08/01/17 22:49 -0800, Tim Shen wrote:
>>
>> On Tue, Jan 3, 2017 at 6:17 AM, Jonathan Wakely <jwak...@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
