[libcxx-commits] [PATCH] D148416: [libc++][spaceship] Implement `operator<=>` for `multiset` and `set`

[Hristo Hristov via Phabricator via libcxx-commits]

H-G-Hristov added a comment.

@Mordante Thank you for the comments. Sorry about answering your question with questions though.



================
Comment at: libcxx/include/set:1044
+template <class _Key, class _Allocator>
+inline _LIBCPP_HIDE_FROM_ABI __synth_three_way_result<_Key>
+operator<=>(const set<_Key, _Allocator>& __x, const set<_Key, _Allocator>& __y) {
----------------
Mordante wrote:
> 
@Mordante Could you please elaborate why I need to remove `inline`? I am asking because `operator<=>` for `map`, `list`, `forward_list`, `deque` are also `inline` as well as the remaining operators?
If I need to remove `inline` here, do I need to also update the other containers for consistency?


================
Comment at: libcxx/test/std/containers/associative/multiset/multiset.nonmember/compare.three_way.pass.cpp:24
+int main(int, char**) {
+  assert(test_ordered_set_container_spaceship<std::multiset>());
+  // `std::multiset` is not constexpr, so no `static_assert` test here.
----------------
Mordante wrote:
> Does this also test containers where `Compare != std::less` ?
No. I didn't consider the other cases. What other cases would you like me to add?
For consistency, I will also need to update the tests for `map` in that case.


================
Comment at: libcxx/test/support/test_container_comparisons.h:233
+  {
+    Container<Elem> l1{1, 2, 2, 4};
+    Container<Elem> l2{1, 1, 2, 3};
----------------
Mordante wrote:
> Does this work with a `set`?
To reduce code duplication I selected the values to work for `set` and `multiset`. I did the same for `map` and `multimap` before.
Or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148416