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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
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) {
----------------
H-G-Hristov wrote:
> 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?
You can keep it if you want, but templates are implicitly inline. Sometimes we forget to point it out in reviews.


================
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.
----------------
H-G-Hristov wrote:
> 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.
For the generic comparison it would be good to make sure the Compare does not matter. We probably should also test that maps with different Compare or Alloc can't be compared.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148416