[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 6 inline comments as done.
Quuxplusone added a comment.

@ldionne ping — some responses to your comments; no planned changes yet.



================
Comment at: libcxx/include/__compare/partial_order.h:34
+        _LIBCPP_HIDE_FROM_ABI static constexpr auto
+        __go(_Ep&& __e, _Fp&& __f, __priority_tag<2>)
+            noexcept(noexcept( partial_ordering(partial_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f))) ))
----------------
ldionne wrote:
> That speaks a bit more than `go` (even though I'm pretty sure I'm guilty of this name at some point too, I have a faint memory).
FWIW, I wanted to stay away from "overloading" names that already have a different meaning in the STL. What we're doing here is very much not `apply`'ing anything (in the `std::apply` sense); we just need a name that is both a reasonable substitute for `operator()`, and reasonably Ctrl+F'able (so I also didn't want `__f`).  I also have a faint memory of `__go` somewhere in libc++, but `git grep` says it's not there now.

Still, I think it would be nice for us to standardize on `__go`, for "overload sets like this that take `__priority_tag`"!


================
Comment at: libcxx/include/__compare/partial_order.h:66
+        {
+            return             __go(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f), __priority_tag<2>());
+        }
----------------
ldionne wrote:
> Do you want to ADL-protect those?
You mean `__go`? No, it's a member, so it needn't (and can't) be qualified.


================
Comment at: libcxx/include/__compare/strong_order.h:40
+        _LIBCPP_HIDE_FROM_ABI static constexpr auto
+        __go(_Ep&& __e, _Fp&& __f, __priority_tag<2>)
+            noexcept(noexcept( strong_ordering(strong_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f))) ))
----------------
ldionne wrote:
> Throughout: I've found it a bit surprising that the parameters are called `e` and `f`. I would've found `a` and `b` easier to follow and closer to what everyone does. Any reason?
The Standard calls them `E` and `F`, so I just went with that.
https://eel.is/c++draft/cmp.alg#1
I suspect the original etymology is just "`E`xpression". :)
I'm not super strongly attached, but if we were to change them, I think I'd want `_Tp __t, _Up __u` over either `_Tp __a, _Up __b` (inconsistent) or `_Ap __a, _Bp __b` (unfamiliar).


================
Comment at: libcxx/include/__compare/strong_order.h:58
+                return (__rx <=> __ry);
+            } else if constexpr (numeric_limits<_Dp>::is_iec559 && sizeof(_Dp) == sizeof(int64_t)) {
+                int64_t __rx = _VSTD::bit_cast<int64_t>(__e);
----------------
ldionne wrote:
> So for `long double`, we're supposed to implement it using the ISO/IEC/IEEE 60559 algorithm but currently we fall back to the common `operator<` & friends. Are those equivalent, but the ISO/IEC/IEEE 60559 is simply more efficient? If that's the case, I think it's reasonable not to implement it immediately. However, if the current implementation isn't correct on `long double`, then we either can't ship it or we should error out explicitly if someone uses `long double`.
"Are those equivalent[?]" — No; non-IEC559 types can't guarantee to order sNAN before qNAN, because we don't know anything about the bit layout of the type.  //However//, what actually happens in this case is that we go to codegen the long `else if` ladder, and we get down to the `bit_cast` on line 101, and it blows up at compile-time because we're trying to `bit_cast` to `void` (line 93).
So, if `sizeof(long double) == sizeof(double)`, then I think we do the right thing; and if `sizeof(long double) > sizeof(double)` (as on common platforms), //regardless// of whether it's IEC559 or not, then we already error out at compile time rather than do the wrong thing.

If you like, I could add a more explicit assertion, like
```
} else if constexpr (!(sizeof(_Dp) == sizeof(int32_t) || sizeof(_Dp) == sizeof(int64_t))) {
    static_assert(sizeof(_Dp) == 0, "Only 32-bit and 64-bit floating-point types are supported right now");
```


================
Comment at: libcxx/include/__compare/weak_order.h:46
+        __go(_Ep&& __e, _Fp&& __f, __priority_tag<2>) noexcept
+        {
+            std::partial_ordering __po = (__e <=> __f);
----------------
ldionne wrote:
> What am I missing here? We're never checking for `is_iec559`?
The `weak_order` algorithm doesn't benefit from knowing it's IEC559.
https://eel.is/c++draft/cmp.alg#2
We can't just compare the bits int-wise, the way we can for `strong_order`, because we need to arrange it so that `std::weak_order(qNAN, sNAN) == std::weak_ordering::equivalent` even when their bits are different. That motivates lines 55–66. And then the rest of the algorithm ends up being exactly the same no matter whether the type is IEC559 or not (because it's just the ordinary meaning of `<=>`, including the ordinary meaning of equivalence for positive+negative zeroes).


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:111
+template<class F>
+constexpr bool test_1_3()
+{
----------------
ldionne wrote:
> General comment for all the floating point comparisons in this patch: is there an existing set of inputs with known comparison outputs we could use and feed as test data? IOW, if we had a list of floating points and how they compare to each other, we could use that to validate our implementation -- as long as we satisfy that almost exhaustive set of inputs, we'd know our algorithm is most likely implemented correctly.
I don't know of an //existing// "test vector" for floating-point; but I claim that my lines 123–134 are pretty good if I do say so myself. ;) I'm covering INF, the whole range of normals, and positive and negative zero. The one hole, I guess, is that I'm not doing anything with subnormals (numbers whose magnitude is less than `numeric_limits<F>::min()`). I don't know how to generate such numbers, except through `bit_cast`. (But that is my own ignorance; there probably is a way I just don't know.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738