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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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 at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:37
 // then skips the iterator comparisons inside the loop.
 template <class _InputIterator1, class _InputIterator2, class _Cmp>
 _LIBCPP_HIDE_FROM_ABI constexpr auto __lexicographical_compare_three_way_fast_path(
----------------
I think it would be better to have the changes to this file and the changes directly related to it in a separate commit.

If you feel strongly about not doing that, then mention these changes in the commit message.


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/compare.three_way.pass.cpp:12
+
+// template<class T, class Allocator>
+//   constexpr synth-three-way-result<T> operator<=>(const vector<T, Allocator>& x,
----------------
The synopsis should use the synopsis for the `vector<bool>` specialization.


================
Comment at: libcxx/test/support/test_container_comparisons.h:65
 constexpr bool test_sequence_container_spaceship() {
+  // Thanks to SFINAE, the following is not a compiler error but returns `false`
+  struct NonComparable {};
----------------
All changes in this file are just NFC cleanups right? If so I prefer to remove them from this patch and commit them separately.
(That commit doesn't need a review.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132268