[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/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
----------------
Quuxplusone wrote:
> mumbleskates wrote:
> > Quuxplusone wrote:
> > > @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.
> > Yeah, this all makes sense. I was mostly thinking in terms of getting this to also run statically, or check at our `strong_ordering` header's compile time whether these basic assumptions about the natural ordering of float payloads hold. Again not sure whether that's within our mandate, but recall that the iec559 standard //recommends//, but does not //require//, that the signaling bit be set to 1 for quiet and 0 for signaling; it //may// be reversed.
> > 
> > So yes, this also depends on how paranoid we are being. The only arch I have read about doing this for sure is PA-RISC, whose most recently released CPU came out in 2005, and any support for that platform is "out-of-tree" for LLVM.
> I think that adding `static_assert` to our actual `libcxx/include/` headers would be a bad idea. Our `libcxx/test/` tests will certainly catch such an issue, on any supported platform; and I would hope that anyone trying to use libc++ on a non-supported platform would at least think to run the tests first, just out of curiosity.
> 
> I did notice on Godbolt that apparently MSVC-on-Windows uses `0x7fc00000` for quiet-NaN (as usual) but `0x7fc00001` for signaling-NaN (which is not a signaling NaN on the platforms I'm familiar with).
> https://godbolt.org/z/oh6ch1dGa
Okay that makes sense. If there are really that many buggy underlying implementations of `quiet_NaN` and `signaling_NaN` we're an order of magnitude more likely to see those just be outright wrong in this way than we are to find platforms with different-looking NaNs then it isn't really very helpful for us to guard against it this way. Sounds good


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738