[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 requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


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


================
Comment at: libcxx/include/__compare/partial_order.h:66
+        {
+            return             __go(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f), __priority_tag<2>());
+        }
----------------
Do you want to ADL-protect those?


================
Comment at: libcxx/include/__compare/partial_order.h:38
+        {
+            return             partial_ordering(partial_order(_VSTD::forward<_Ep>(__e), _VSTD::forward<_Fp>(__f)));
+        }
----------------
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; }

```


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


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


================
Comment at: libcxx/include/__compare/weak_order.h:46
+        __go(_Ep&& __e, _Fp&& __f, __priority_tag<2>) noexcept
+        {
+            std::partial_ordering __po = (__e <=> __f);
----------------
What am I missing here? We're never checking for `is_iec559`?


================
Comment at: libcxx/include/__utility/priority_tag.h:21-22
+
+template<size_t _Ip> struct _PriorityTag : _PriorityTag<_Ip - 1> {};
+template<> struct _PriorityTag<0> {};
+
----------------
Quuxplusone wrote:
> It probably makes sense to rename this from `_PriorityTag` to `__priority_tag`, especially in light of the `_EnableIf` renaming. @ldionne, opinion?
Yeah, I agree.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:111
+template<class F>
+constexpr bool test_1_3()
+{
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738