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


================
Comment at: libcxx/include/__compare/partial_order.h:38
+        {
+            return             partial_ordering(partial_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f)));
+        }
----------------
ldionne wrote:
> ldionne wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > Mordante wrote:
> > > > > 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?
> > > > This is our standard formatting now for "you must write it three times" functions; see D108144. (Which came out of discussion on some other review.)
> > > > We guard it against clang-format in the same way as we guard the entire rest of libc++: don't run clang-format on these files, and if someone accidentally (or just naively) does, then we hope to catch it in code review.
> > > > (We could start wrapping whole files in `clang-format off` comments, but: That's an O(n) solution to an O(1) problem. And, it works only because `clang-format` is a monoculture. If anyone ever writes a second C++ formatter, we'd have to guard against //that// one, too. To really ensure that nobody can mess with our aesthetics, we'd put in some sort of technological "do-not-sed" and "do-not-awk" protection... I'm having flashbacks to DeCSS here... ;))
> > > Fair point. I recall some of these discussions and the nice link you posted about a lightning talk regarding this issue.
> > As a side note, usually we do this:
> > 
> > ```
> > noexcept(noexcept(EXPRESSION))
> > -> decltype(      EXPRESSION)
> > { return          EXPRESSION; }
> > 
> > ```
> Ping on this - please format consistently with the other places where we do this. It helps for grepping.
The difference was just the extra spaces I put around `noexcept(noexcept( E ))` and `decltype( E )`, right? Is this the way you want it now?


================
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:
> Quuxplusone wrote:
> > 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).
> Non-binding, but I'd like to use `__t` and `__u` if we can agree on that. We don't use `__e` very often, and we are pretty consistent about using `__f` to denote some sort of callable, which I find misleading here.
Done. (But it's easy to revert if anyone objects.)


================
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:
> Quuxplusone wrote:
> > 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");
> > ```
> I'd be happy with the assertion.
Added a "temporary, TODO FIXME" .verify.cpp test to verify the current behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738