[libcxx-commits] [PATCH] D110738: [libc++] [P1614] Implement the first half of [cmp.alg]: strong_order, weak_order, partial_order.

Tue Nov 16 07:21:04 PST 2021

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

LGTM with requested changes and CI passing. I think you had updates to the Status pages you wanted to do separately from this commit -- please don't forget to land those (I don't mind them being landed in the same commit FWIW).

================
Comment at: libcxx/docs/Status/Cxx20Issues.csv:253
 "`3329 <https://wg21.link/LWG3329>`__","``totally_ordered_with``\  both directly and indirectly requires ``common_reference_with``\ ","Prague","|Complete|","13.0"
-"`3330 <https://wg21.link/LWG3330>`__","Include ``<compare>``\  from most library headers","Prague","","","|spaceship|"
+"`3330 <https://wg21.link/LWG3330>`__","Include ``<compare>``\  from most library headers","Prague","|Complete|","13.0","|spaceship|"
 "`3331 <https://wg21.link/LWG3331>`__","Define ``totally_ordered/_with``\  in terms of ``partially-ordered-with``\ ","Prague","|Complete|","13.0"
----------------
Do you mean 14.0 or 13.0 here?

================
Comment at: libcxx/docs/Status/SpaceshipPapers.csv:5
 `P2405R0 <https://wg21.link/P2405>`_,nullopt_t and nullptr_t should both have operator<=> and operator==,,
-`LWG3330 <https://wg21.link/LWG3330>`_,Include <compare> from most library headers,|In Progress|,
+`LWG3330 <https://wg21.link/LWG3330>`_,Include <compare> from most library headers,"|Complete|","13.0"
 `LWG3347 <https://wg21.link/LWG3347>`_,"std::pair<T, U> now requires T and U to be less-than-comparable",|Nothing To Do|,
----------------
Same question, 14.0 or 13.0?

================
Comment at: libcxx/include/__compare/partial_order.h:26
+
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR) && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
----------------
`_LIBCPP_HAS_NO_SPACESHIP_OPERATOR` can be removed now.

================
Comment at: libcxx/include/__compare/partial_order.h:38
+        {
+            return             partial_ordering(partial_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f)));
+        }
----------------
Quuxplusone wrote:
> ldionne wrote:
> > ldionne wrote:
> > > Mordante wrote:
> > > > Quuxplusone wrote:
> > > > > Mordante wrote:
> > > > > > I like this manually formatting a lot! Would it make sense to guard this code against clang-format, just in case we want to enable it for the entire libc++ in the future?
> > > > > This is our standard formatting now for "you must write it three times" functions; see D108144. (Which came out of discussion on some other review.)
> > > > > We guard it against clang-format in the same way as we guard the entire rest of libc++: don't run clang-format on these files, and if someone accidentally (or just naively) does, then we hope to catch it in code review.
> > > > > (We could start wrapping whole files in `clang-format off` comments, but: That's an O(n) solution to an O(1) problem. And, it works only because `clang-format` is a monoculture. If anyone ever writes a second C++ formatter, we'd have to guard against //that// one, too. To really ensure that nobody can mess with our aesthetics, we'd put in some sort of technological "do-not-sed" and "do-not-awk" protection... I'm having flashbacks to DeCSS here... ;))
> > > > Fair point. I recall some of these discussions and the nice link you posted about a lightning talk regarding this issue.
> > > As a side note, usually we do this:
> > > 
> > > ```
> > > noexcept(noexcept(EXPRESSION))
> > > -> decltype(      EXPRESSION)
> > > { return          EXPRESSION; }
> > > 
> > > ```
> > Ping on this - please format consistently with the other places where we do this. It helps for grepping.
> The difference was just the extra spaces I put around `noexcept(noexcept( E ))` and `decltype( E )`, right? Is this the way you want it now?
Sorry, should have made it clearer, but the difference is the positioning of braces. This seems like a nitpick, but I've mass-modified these expressions in the past and having consistent formatting really helps.

================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order_long_double.verify.cpp:16-18
+// libc++ does not support strong_order(long double, long double) quite yet.
+// This test verifies the error message we give for that case.
+// TODO: remove this test once long double is properly supported.
----------------
Quuxplusone wrote:
> This test fails on arm7 (32-bit?) and Win32, where `long double` is 64 bits just like `double`.
> It also fails on arm8 (64-bit?) for some reason I can't reproduce on Godbolt.
> It also fails on AIX.
> @ldionne: Any idea what `XFAIL` lines I should add here? Or shall I just eliminate this test again?
What I did to figure this out is go to the various CI jobs that were failing and look for the following line in the Lit output: `add Lit feature target=x86_64-pc-windows-msvc' as a result of parameter 'target_triple=x86_64-pc-windows-msvc'` (and similarly for all failing platforms). I think what you want is:

```
// The following platforms have sizeof(long double) == sizeof(double), so this test doesn't apply to them.
// UNSUPPORTED: target={{arm64|armv8|armv7|powerpc|powerpc64}}-{{.+}}
// UNSUPPORTED: target=x86_64-pc-windows-{{.+}}
```

================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order_long_double.verify.cpp:24-28
+int main(int, char**)
+{
+    long double ld = 3.14;
+    (void)std::strong_order(ld, ld);  // expected-error@*:* {{std::strong_order is unimplemented for this floating-point type}}
+}
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738