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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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:
> 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.


================
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))) ))
----------------
Quuxplusone wrote:
> 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`"!
Alright, I still prefer `__apply` but won't bikeshed here since it's mostly about personal preference. Let's agree on not having any standardized name for this for now :-).


================
Comment at: libcxx/include/__compare/partial_order.h:66
+        {
+            return             __go(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f), __priority_tag<2>());
+        }
----------------
Quuxplusone wrote:
> 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.
Reviewed my ADL 101 and you're right.

Note that you can, however, qualify it. Like `this->__go` or `__fn::__go`. No action needed.


================
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))) ))
----------------
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.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738