[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)
----------------
Quuxplusone wrote:
> mumbleskates wrote:
> > 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.
> `constexpr` globals, like `const` globals, are "implicitly `static` by default." So `inline` is observable for public STL globals like `std::nullopt`, because you can say things like
> ```
> void print_address() { printf("%p\n", (void*)&std::nullopt); }
> ```
> and it needs to come out to the same address in every translation unit.
> For internal implementation details like `__synth_three_way`, I cannot think of any way that the difference between (implicitly `static`) and (explicitly `inline`) could be observed by the user... but, if the library itself were to use `__synth_three_way` inside another inline function like
> ```
> inline void some_other_library_thing() { ~~~ __synth_three_way(...) ~~~ }
> ```
> then //that// would technically be an ODR violation, because the two different definitions of `inline void some_other_library_thing()` would end up referring to //different// internal-linkage `__synth_three_way`s, and that's an ODR violation. So yes, `inline constexpr` SGTM.
> 
> I don't really understand `_LIBCPP_HIDE_FROM_ABI` either, but we put it on everything, so, it's a good idea. ;)
Cool that makes sense. I wasn't really thinking about it in terms of a value but I guess it technically could have an address in an unoptimized build and it's better to just avoid that.


================
Comment at: libcxx/include/__utility/pair.h:314
 
 template <class _T1, class _T2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > Before this line, I'd appreciate the addition of a comment line just for ease of comparing against the Standard:
> > ```
> > // [pairs.spec], specialized algorithms
> > ```
> You added the comment line on 323, but I think it should be moved upward to ~line 312. `operator==` is also part of [pairs.spec].
Oh good call, I forgot about that.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
mumbleskates wrote:
> 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.
I do want to clean this up and address this better if we want to figure out a consistent ground rule to base the tests on, including a full list of what behaviors need to be tested where and making sure they all get covered somewhere. There are at least 8 more places other than `pair` where `synth-three-way` is used so I want to make sure we're testing the right surface without introducing too much fragility if we find new cases that need fixing in the future. However this might be best suited for a follow-up diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721