[libcxx-commits] [PATCH] D139554: [libc++] Forward to std::memcmp for trivially comparable types in equal

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__type_traits/is_comparable.h:37
+                        __is_equality_comparable<_Tp, _Up>::value && is_integral<_Tp>::value &&
+                            is_same<__remove_cv_t<_Tp>, __remove_cv_t<_Up> >::value> {};
+
----------------
Mordante wrote:
> philnik wrote:
> > Mordante wrote:
> > > This looks really odd, and it would be nice to have comment why this design is chosen. 
> > > Why are the following types not in the set, floating-point values, enums, pointers.
> > > 
> > > Is `__is_trivially_equality_comparable<int, float>` true? The other way around isn't.
> > > 
> > > I really want some internal tests for this type trait.
> > > This looks really odd, and it would be nice to have comment why this design is chosen.
> > Could you elaborate on what's really odd about this design?
> > > Why are the following types not in the set, floating-point values, enums, pointers.
> > `floating-point`: `0.f == -0.f` is true, but they have different binary representations.
> > `enum`: AFAIK you are allowed to define your own `operator==` for enums, so we don't know that `UserEnum::Value1 == UserEnum::Value2` is actually the same as comparing their binary representations unless we have compiler support. (That would even allow us to optimize for structs that have `bool operator==(const Struct&) = default;`)
> > `pointer`s: are added through the specialization in line 40.
> > > Is `__is_trivially_equality_comparable<int, float>` true? The other way around isn't.
> > No, `is_same_v<inf, float>` is false.
> > > I really want some internal tests for this type trait.
> > I'll add some.
> Thanks I see I missed the `is_same`. I still feel it would be good to add some comment to this file what you mean with trivially equality comparable. It's not a "normal" term and it may be misleading
> ```
> struct foo {
>   int bar;
>   bool operator==(const foo&) = default;
> };
> ```
> This struct is trivial, but not trivially equality comparable. I would not be surprised to see a paper proposing a similar type trait.
> 
> 
I would actually consider your example to be trivially equality comparable, since `memcmp` would give the correct result. It's just that we can't detect this as trivially equality comparable without compiler support. (kind-of a tangent: would it be possible to detect with reflection? That would be really neat.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139554