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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates 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)
----------------
ldionne wrote:
> 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.
It appears to work just fine if the lambda is defined as `_LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way = []<...` but it's not completely clear to me what effect this has.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> 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).
I think `synth-three-way` is pretty well "mentioned" in the spec, and its behavior pretty thoroughly described there; I would argue that the only "implementation detail" we are depending on here is what the functor is actually named.

I can see an argument for moving the `synth-three-way` test out of `test/libcxx/library` and into `test/std` somewhere, that would be fine. It's my personal preference to test the minutiae of how `synth-three-way` actually behaves in that file, and to test that usages of the functor in the publicly exposed standards fall back on it correctly without worrying about every possible corner case.

Is there a good way to determine coverage as you mentioned? Does this include conditions in templates and SFINAE testing? The concept of "coverage" here is pretty complex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721