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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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)));
+        }
----------------
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.


================
Comment at: libcxx/include/__compare/strong_order.h:96
+                >;
+                static_assert(!std::is_same_v<_IntType, void>, "std::strong_order is unimplemented for this floating-point type");
+                if (__e_is_nan && __f_is_nan) {
----------------
Quuxplusone wrote:
> Mordante wrote:
> > I'm not familiar with this floating point code. But wouldn't this assert trigger on system where `sizeof(int) == 8 && sizeof(float) == 4`. Isn't it possible to test with `int32_t` and `int64_t`?
> This assert //would// trigger on such systems, if any such systems exist.
> On the one hand, I can't think of any reason for `int` to be 64-bit, unless `char` is also 64-bit, in which case `float` must also be at least 64-bit. So it makes more sense to wait and see if any such systems use libc++, and then if they do, they can submit a patch to use (i.e. `conditional_t` against) their particular type that works.
> On the other hand, I actually //did// use `int32_t` and `int64_t` on lines 52–58, so why do I switch back to the built-in types here? I forget. It's particularly weird that I'm testing //three// types when in practice they'll only have //two// different sizes (32 and 64). So probably I should just change this to test only `int32_t` and `int64_t`.
> I dunno; anyone else want to be the tiebreaker?
I prefer the `int32_t` and `int64_t` especially since you used that earlier in the code.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:448
+    static_assert(test_1_3<double>());
+    // static_assert(test_1_3<long double>());  // UNIMPLEMENTED
+    static_assert(test_1_4());
----------------
Quuxplusone wrote:
> @Mordante writes:
> > For long double would it be possible to use an `__int128_t` when available? (Obviously this can't be `bit_cast`-ed.)
> 
> Yeah, `long double` on x86-64 has 80 value bits and 48 padding bits, so it can't be bit-casted straight to 128-bit `__int128_t`. We //can// bit-cast it to an array of char and then inspect the bytes individually; but then we have to be really confident about how they're arranged in memory (endianness? which side does the padding wind up on?) and I haven't done that homework.
> Orthogonally, on non-IEC559 (non-IEEE754) platforms like I believe PPC64, `long double` is actually represented in memory as a //pair// of IEEE754 `double` values with different exponents, and then to find the `long double`'s value, you add them together. In which case I haven't even bothered to think about what the correct `strong_order` algorithm would look like. (It might be exactly what I've got for the non-IEEE754 codepath, and then still tiebroken by "bit_cast and memcmp." But I haven't thought about it.) 
> So I would prefer to leave `long double` unimplemented in //this// PR; and then either we solicit an implementation from someone who claims to know what they're doing, or else eventually I get bored and submit something (and then we wait for //bugfixes// from someone who knows what they're doing). :)
> So I would prefer to leave `long double` unimplemented in //this// PR; and then either we solicit an implementation from someone who claims to know what they're doing, or else eventually I get bored and submit something (and then we wait for //bugfixes// from someone who knows what they're doing). :)

Sounds fine to me to not do `long double` in this PR. That sounds like the SO method, do it wrong and wait for people pointing out why it's wrong ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738