[libcxx-commits] [PATCH] D107721: [libc++][spaceship] Implement std::pair::operator<=>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__compare/synth_three_way.h:28
+
+constexpr auto __synth_three_way =
+  []<class _Tp, class _Up>(const _Tp& __t, const _Up& __u)
----------------
I believe this needs to be `_LIBCPP_HIDE_FROM_ABI inline`. The `inline` is to avoid ODR violations (we're defining a global in a header!) and the `_LIBCPP_HIDE_FROM_ABI` is to avoid the symbol leaking into the ABI of clients (inline variables result in weak defs being exported from programs that use them, which is pretty bad).

I don't have enough time to investigate the details right now but I could do that tomorrow.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
If we remove `libcxx/test/libcxx/library/description/conventions/expos.only.func/synth_three_way.pass.cpp`, do we lose any coverage? (I assume the answer is yes). If so, is it possible to add that test coverage in `libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp` without mentioning implementation details of libc++ instead?

In libc++, we've historically written tests using only what the Standard mentions, and `libcxx/test/libcxx` is typically for testing libc++ specific behavior, but not for testing libc++ implementation details (we don't generally test those, or if we do, we still aim for 100% test coverage of the standard facility).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721