Implements part of P1614R2 "The Mothership has Landed"
Depends on D150188
Differential D132268
[libc++][spaceship] Implement `operator<=>` for `vector` H-G-Hristov on Aug 19 2022, 4:52 PM. Authored by
Details
Implements part of P1614R2 "The Mothership has Landed" Depends on D150188
Diff Detail
Event TimelineComment Actions needs to be adjusted to use the still-to-be-introduced test utilities from https://reviews.llvm.org/D132312 is done Comment Actions Incorporate feedback received for similar review (list instead of vector; https://reviews.llvm.org/D132312) Comment Actions I had a quick look and no real comments. Can you fix the CI? I prefer to review the changes after a CI run. Comment Actions @H-G-Hristov unfortunately, I don't know when I will find time to come back to this (and https://reviews.llvm.org/D132265). Feel free to take this over, if you are blocked by it Comment Actions Thank you. I'll probably take them over then if you haven't finished them before I get the time. Comment Actions
Comment Actions @Mordante Thank you for the review.
Comment Actions Looks basically good, but I'd like to understand the __debug_three_way_comp change before approving.
Comment Actions @philnik Thank you for the review! This change was proposed by avogelsang above actually, as without it the tests of vector<bool> build fails with:
Comment Actions 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. Comment Actions 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( Comment Actions 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. 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? Comment Actions Could you please confirm if I understand correctly: I am supposed to perfectly forward the arguments to std::__lexicographical_compare_three_way_fast_path something like that: return std::__lexicographical_compare_three_way_fast_path( std::forward<_InputIterator1>(__first1), std::forward<_InputIterator1>(__last1), std::forward<_InputIterator2>(__first2), std::forward<_InputIterator2>(__last2), __wrapped_comp_ref); No, that still wasn't enough. The compilation fails without changes to LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __x, _Up& __y)'s signature. There was ways an issue with the 5th parameter (for the record I tried forward, move). Comment Actions I meant that you should perfect-forward the arguments in __debug_three_way_comp::operator(). i.e. std::forward the arguments to __comp_. The std::forwards you are currently doing in the calls are essentially just std::moves, since the arguments are prvalue types. Comment Actions A few more comments. I want to have a short look at the final version especially the answers to some of @philnik's remarks.
Comment Actions @Mordante Thank you for the suggestions!
Comment Actions Thank you for the review. This one now passes but it still needs the stacked change to be completed/approved. |
Did you validate this output looks good?