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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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)
----------------
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. ;)


================
Comment at: libcxx/include/__utility/pair.h:332
+{
+    if (auto __c = _VSTD::__synth_three_way(__x.first, __y.first); __c != 0) return __c;
+    return _VSTD::__synth_three_way(__x.second, __y.second);
----------------
mumbleskates wrote:
> mumbleskates wrote:
> > Mordante wrote:
> > > Please place `return __c;` on a new line. Not sure whether this contradicts @cjdb's comment, but in libc++ we normally don't put an `if` and a `return` on one line.
> > Yes this was written to be spelled exactly as it appears in the standard, so that does contradict.
> Reference: https://eel.is/c++draft/pairs.spec#2
FWIW, I would also prefer to see the `return` on its own line. I don't think "spell it exactly as in the standard" means "all the way down to the whitespace."


================
Comment at: libcxx/include/__utility/pair.h:314
 
 template <class _T1, class _T2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
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].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721