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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM, modulo two small formatting issues. Also a suggestion to improve the tests on non GCC platforms.



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


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:15
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, libcpp-no-concepts
+
----------------
We normally start with this line, can you move it above `// <utility>`?


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:96
+
+  // GCC cannot evaluate NaN @ non-NaN constexpr, so test that runtime-only.
+  {
----------------
How do you feel about this work-around and move these two tests back in `test()`?
When you do that make sure to start a CI run before committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721