[libcxx-commits] [PATCH] D132268: [libc++][spaceship] Implement `operator<=>` for `vector`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D132268#4321925 <https://reviews.llvm.org/D132268#4321925>, @H-G-Hristov wrote:

> @philnik Thank you for the review!
>
> In D132268#4320495 <https://reviews.llvm.org/D132268#4320495>, @philnik wrote:
>
>> Looks basically good, but I'd like to understand the `__debug_three_way_comp` change before approving.
>
> This change was proposed by avogelsang above actually, as without it the tests of `vector<bool>` build fails with:
>
>> lexicographical_compare_three_way.h:103:12: error: no matching function for call to '__lexicographical_compare_three_way_fast_path'
>>
>>   return std::__lexicographical_compare_three_way_fast_path(
>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> vector:3296:17: note: in instantiation of function template specialization 'std::lexicographical_compare_three_way<std::__bit_iterator<std::vector<bool>, true, 0>, std::__bit_iterator<std::vector<bool>, true, 0>, std::strong_ordering (*)(const bool &, const bool &)>' requested here
>>
>>   return std::lexicographical_compare_three_way(

Ah OK. It's for iterators not returning an lvalue reference. In this case I think it would be better to forward the arguments, since that will definitely be correct.

In D132268#4322470 <https://reviews.llvm.org/D132268#4322470>, @H-G-Hristov wrote:

> In D132268#4322098 <https://reviews.llvm.org/D132268#4322098>, @philnik wrote:
>
>> In D132268#4321925 <https://reviews.llvm.org/D132268#4321925>, @H-G-Hristov wrote:
>>
>>> @philnik Thank you for the review!
>>>
>>> In D132268#4320495 <https://reviews.llvm.org/D132268#4320495>, @philnik wrote:
>>>
>>>> Looks basically good, but I'd like to understand the `__debug_three_way_comp` change before approving.
>>>
>>> This change was proposed by avogelsang above actually, as without it the tests of `vector<bool>` build fails with:
>>>
>>>> lexicographical_compare_three_way.h:103:12: error: no matching function for call to '__lexicographical_compare_three_way_fast_path'
>>>>
>>>>   return std::__lexicographical_compare_three_way_fast_path(
>>>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> vector:3296:17: note: in instantiation of function template specialization 'std::lexicographical_compare_three_way<std::__bit_iterator<std::vector<bool>, true, 0>, std::__bit_iterator<std::vector<bool>, true, 0>, std::strong_ordering (*)(const bool &, const bool &)>' requested here
>>>>
>>>>   return std::lexicographical_compare_three_way(
>>
>> Ah OK. It's for iterators not returning an lvalue reference. In this case I think it would be better to forward the arguments, since that will definitely be correct.
>
> Sorry, I think I confused you. The actual problem is in the compare function, the last parameter of: :
>
>   note: candidate template ignored: substitution failure [with _InputIterator1 = std::__bit_iterator<std::vector<bool>, true, 0>, _InputIterator2 = std::__bit_iterator<std::vector<bool>, true, 0>, _Cmp = std::__debug_three_way_comp<std::strong_ordering (*)(const bool &, const bool &)>]: no matching function for call to object of type 'std::__debug_three_way_comp<std::strong_ordering (*)(const bool &, const bool &)>'
>   _LIBCPP_HIDE_FROM_ABI constexpr auto __lexicographical_compare_three_way_fast_path(



In D132268#4322470 <https://reviews.llvm.org/D132268#4322470>, @H-G-Hristov wrote:

> In D132268#4322098 <https://reviews.llvm.org/D132268#4322098>, @philnik wrote:
>
>> In D132268#4321925 <https://reviews.llvm.org/D132268#4321925>, @H-G-Hristov wrote:
>>
>>> @philnik Thank you for the review!
>>>
>>> In D132268#4320495 <https://reviews.llvm.org/D132268#4320495>, @philnik wrote:
>>>
>>>> Looks basically good, but I'd like to understand the `__debug_three_way_comp` change before approving.
>>>
>>> This change was proposed by avogelsang above actually, as without it the tests of `vector<bool>` build fails with:
>>>
>>>> lexicographical_compare_three_way.h:103:12: error: no matching function for call to '__lexicographical_compare_three_way_fast_path'
>>>>
>>>>   return std::__lexicographical_compare_three_way_fast_path(
>>>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> vector:3296:17: note: in instantiation of function template specialization 'std::lexicographical_compare_three_way<std::__bit_iterator<std::vector<bool>, true, 0>, std::__bit_iterator<std::vector<bool>, true, 0>, std::strong_ordering (*)(const bool &, const bool &)>' requested here
>>>>
>>>>   return std::lexicographical_compare_three_way(
>>
>> Ah OK. It's for iterators not returning an lvalue reference. In this case I think it would be better to forward the arguments, since that will definitely be correct.
>
> Sorry, I think I confused you. The actual problem is in the compare function, the last parameter of: :
>
>   note: candidate template ignored: substitution failure [with _InputIterator1 = std::__bit_iterator<std::vector<bool>, true, 0>, _InputIterator2 = std::__bit_iterator<std::vector<bool>, true, 0>, _Cmp = std::__debug_three_way_comp<std::strong_ordering (*)(const bool &, const bool &)>]: no matching function for call to object of type 'std::__debug_three_way_comp<std::strong_ordering (*)(const bool &, const bool &)>'
>   _LIBCPP_HIDE_FROM_ABI constexpr auto __lexicographical_compare_three_way_fast_path(

IIUC the problem is that `__bit_iterator::operator*` returns an object instead of a reference, which can't bind to an lvalue reference. If we use perfect forwarding that should fix the problem?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132268/new/

https://reviews.llvm.org/D132268