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

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

Quuxplusone marked an inline comment as done.
Quuxplusone 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) {
----------------
mumbleskates wrote:
> 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?
> Would it also be sensible for us to `static_assert` that `quiet_NaN()` and `signaling_NaN()` are ordered correctly by `strong_order`

Isn't that covered in the tests below? (I'll comment on the exact line.)


================
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.
----------------
mumbleskates wrote:
> 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`).
It appears that we can! https://godbolt.org/z/z163Ges6G
However, this requires a bit of metaprogramming to determine an integral type with the same width as `F`, and that's a bit more complicated than I really want to put into this test file. It would basically be repeating the `conditional_t` chain from lines 91–94 of `strong_order.h`.
```
auto copysign = [](F f, int sign) {
    using I = std::conditional_t<sizeof(F) == sizeof(int32_t), int32_t,
        std::conditional_t<sizeof(F) == sizeof(int64_t), int64_t, void>>;
    I mask = std::bit_cast<I>(F(-0.0));
    if (sign < 0) {
        return std::bit_cast<F>(std::bit_cast<I>(to) | mask);
    } else {
        return std::bit_cast<F>(std::bit_cast<I>(to) & ~mask);
    }
};
```


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:386
+#ifndef TEST_BUGGY_SIGNALING_NAN
+        assert(std::strong_order(ps, pq) == std::strong_ordering::less);
+#endif
----------------
@mumbleskates: This is the exact line that compares `quiet_NaN()` against `signaling_NaN()` (and see line 395 too).
Notice that we're testing "positive quiet NaN" (which might not be //literally// `quiet_NaN()`) against "positive signaling NaN" (which might not be //literally// `signaling_NaN()`), and lines 361 and 370 are testing "negative quiet" against "negative signaling"; we never literally test the `numeric_limits` values //directly//. The Standard doesn't mandate (AFAIK) whether `numeric_limits<F>::quiet_NaN()` should have a positive or negative sign bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738