[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/__algorithm/equal.h:89
+template <class _Iter1, class _Sent1, class _Iter2, class _Sent2, class _Pred, class _Proj1, class _Proj2>
+_LIBCPP_NODISCARD inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __equal_trivial_impl(
+    _Iter1 __first1, _Sent1 __last1, _Iter2 __first2, _Sent2 __last2, _Pred& __comp, _Proj1& __proj1, _Proj2& __proj2) {
----------------
ldionne wrote:
> Since we're about to make clang-format mandatory, I think this should be formatted as
> 
> ```
> _LIBCPP_NODISCARD inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
> bool __equal_trivial_impl(args...) {
>   ...
> }
> ```
> 
> We have a lot of long lists of attributes in libc++ so this is going to be a common occurrence. Leaving the tiny name of the function alone at the end of the line IMO makes this really hard to read. If there's no way to make this work with clang-format built-in, I would do this:
> 
> ```
> _LIBCPP_NODISCARD inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 //
> bool __equal_trivial_impl(args...) {
>   ...
> }
> ```
> 
> If clang-format had an option to prefer breaking after the attributes (or after the return type), that would be better than the status quo.
Actually, it looks like clang-format breaks after the return type if all the arguments fit on the same line. In this case the argument list is just so long that it doesn't fit anymore, so it splits after the parenthesis instead.


================
Comment at: libcxx/include/__type_traits/is_comparable.h:41-42
+// TODO: Use is_pointer_inverconvertible_base_of
+template <class _Tp, class _Up>
+struct __is_trivially_equality_comparable<_Tp*, _Up*>
+    : integral_constant<
----------------
ldionne wrote:
> Can you please explain in a comment why this specialization is the way it is. What are the pitfalls, etc.
I've put this in the general comment about trivially equality comparable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139554