[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/benchmarks/CMakeLists.txt:111
 target_link_options(   cxx-benchmarks-flags-libcxx INTERFACE -nodefaultlibs "-L${BENCHMARK_LIBCXX_INSTALL}/lib" ${SANITIZER_FLAGS})
+target_link_options(   cxx-benchmarks-flags-libcxx INTERFACE -nodefaultlibs "-L${BENCHMARK_LIBCXX_INSTALL}/lib64" ${SANITIZER_FLAGS})
 
----------------
Mordante wrote:
> Why is this needed? Is it portable on all (Linux) platforms?
I'm not actually sure why it's needed, but on my system `libbenchmark.a` is in `lib64` and not `lib`. I don't really see how it wouldn't be portable, since the directory just gets ignored if it doesn't exist. I didn't actually plan to change it in this patch, but it shouldn't be a huge problem.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139554