[PATCH] D147175: [clang] Add __is_trivially_equality_comparable

[Nikolas Klauser via Phabricator via cfe-commits]

philnik added a comment.

In D147175#4251076 <https://reviews.llvm.org/D147175#4251076>, @aaron.ballman wrote:

> One thing I can't quite determine is whether we expect to call this type trait often or not. Are either of the uses in libc++ going to be instantiated frequently? I'm trying to figure out whether we want the current implementation of `HasNonDeletedDefaultedEqualityComparison()` or whether we should bite the bullet up front and add another bit to `CXXRecordDecl::DefinitionData` so we compute this property once as the class is being formed and don't have to loop over all of the methods and bases each time. We do this for other special member functions, as with: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclCXX.h#L695

We currently use `__is_trivially_equality_comparable` only for `std::equal` (and I'm working on using it for `std::find`), and I don't think there are a lot more places where we can use it. It's also only relevant for cases where we have raw pointers. Would the `enable_if` be evaluated, even if the arguments aren't raw pointers in the example below?

  template <class T, class U, enable_if_t<is_same_v<remove_cvref_t<T>, remove_cvref_t<U>> && __is_trivially_equality_comparable(T)>>
  bool equal(T* first1, T* last1, U* first2);
  
  <non-raw-pointer-overload>

If not, then I don't think it makes sense to save the information, since it will most likely not be evaluated more than once or twice per type.



================
Comment at: clang/lib/AST/Type.cpp:2618-2619
+         llvm::all_of(Decl->fields(), [](const FieldDecl *FD) {
+           if (!FD->getType()->isRecordType())
+             return !FD->getType()->isReferenceType();
+           return HasNonDeletedDefaultedEqualityComparison(
----------------
aaron.ballman wrote:
> Why the check for !RecordType? If any field is a reference, we can bail out early, so I think this is better as:
> ```
> if (FD->getType()->isReferenceType())
>   return false;
> if (const auto *RD = FD->getType()->getAsCXXRecordDecl())
>   return HasNonDeletedDefaultedEqualityComparison(RD);
> return true;
> ```
> WDYT?
I don't really have an opinion here. Your version seems reasonable, so I'm going to take it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147175