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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante resigned from this revision.
Mordante added a comment.

For `long double` would it be possible to use an `__int128_t` when available? (Obviously this can't be `bit_cast`-ed.)
I've not looked at the tests in detail, but didn't see any obvious issues.

I'm not familiar enough with the exact details of floating point values, so I haven't validated whether this is correct. Other than that the implementation seems good to me.



================
Comment at: libcxx/include/__compare/partial_order.h:38
+        {
+            return             partial_ordering(partial_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f)));
+        }
----------------
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?


================
Comment at: libcxx/include/__compare/strong_order.h:96
+                >;
+                static_assert(!std::is_same_v<_IntType, void>, "std::strong_order is unimplemented for this floating-point type");
+                if (__e_is_nan && __f_is_nan) {
----------------
I'm not familiar with this floating point code. But wouldn't this assert trigger on system where `sizeof(int) == 8 && sizeof(float) == 4`. Isn't it possible to test with `int32_t` and `int64_t`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738