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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


================
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) {
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > 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`?
> > This assert //would// trigger on such systems, if any such systems exist.
> > On the one hand, I can't think of any reason for `int` to be 64-bit, unless `char` is also 64-bit, in which case `float` must also be at least 64-bit. So it makes more sense to wait and see if any such systems use libc++, and then if they do, they can submit a patch to use (i.e. `conditional_t` against) their particular type that works.
> > On the other hand, I actually //did// use `int32_t` and `int64_t` on lines 52–58, so why do I switch back to the built-in types here? I forget. It's particularly weird that I'm testing //three// types when in practice they'll only have //two// different sizes (32 and 64). So probably I should just change this to test only `int32_t` and `int64_t`.
> > I dunno; anyone else want to be the tiebreaker?
> I prefer the `int32_t` and `int64_t` especially since you used that earlier in the code.
Would it also be sensible for us to `static_assert` that `quiet_NaN()` and `signaling_NaN()` are ordered correctly by `strong_order`, or even perhaps SFINAE via a wrapper function that participates only when the behavior is as expected?


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:348-351
+    // There's no way to produce a specifically positive or negative NAN
+    // at compile-time, so the NAN-related tests must be runtime-only.
+    // Also, x86-32 (x87 floating point) cannot handle signaling NANs;
+    // see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57484 for context.
----------------
Now that we have `bit_cast`, is it not possible for us to flip the sign by round tripping it it through an integer and xor'ing it with the bits for `-0.0` at compile time? (I am pretty sure that -0.0 has, in all iec559 encodings binary and decimal, only the sign bit set and all others zeroed out, which can also be checked via `has_single_bit`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738