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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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})
 
----------------
philnik wrote:
> 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.
I've no strong objection against it, if breaks the benchmarks for users we can revert it.


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:69
+         __enable_if_t<!is_copy_constructible<_Iter>::value, int> = 0>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
+_Iter __unwrap_iter(_Iter __i) _NOEXCEPT {
----------------
This will only be used in C++20 or later.


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:70
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
+_Iter __unwrap_iter(_Iter __i) _NOEXCEPT {
+  return __i;
----------------



================
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> {};
+
----------------
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.




================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_comparable.compile.pass.cpp:37
+
+static_assert(!std::__is_trivially_equality_comparable<float, int>::value, "");
+static_assert(!std::__is_trivially_equality_comparable<double, long long>::value, "");
----------------
Please add `static_assert(!std::__is_trivially_equality_comparable<float, float>::value, "");` test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139554