[PATCH] D147175: [clang] Add __is_trivially_equality_comparable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D147175#4249212 <https://reviews.llvm.org/D147175#4249212>, @philnik wrote:

> In D147175#4246513 <https://reviews.llvm.org/D147175#4246513>, @aaron.ballman wrote:
>
>> Ah, I like this approach -- it keeps things roughly in sync with checking for unique object representations, which is great. I spotted a few questions, but in general this is heading in the right direction. You should also add a release note to clang/docs/ReleaseNotes.rst so that users know there's a new builtin coming.
>>
>> I suppose one thing to double-check: do you know of any other STL implementations that might be using this identifier as part of their implementation details? (We've had issues in the past where we accidentally stole a reserved identifier that was being used by libstdc++ and it caused issues.)
>
> Neither libstdc++ nor the MSVC STL use the `__is_trivially_equality_comparable` AFAICT. A sourcegraph search also exclusively finds libc++: https://sourcegraph.com/search?q=context%3Aglobal+__is_trivially_equality_comparable&patternType=standard&sm=1&groupBy=repo (which only started using it in this release cycle, so it shouldn't be a problem).

Perfect, thank you for double-checking!

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



================
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(
----------------
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?


================
Comment at: clang/lib/AST/Type.cpp:2631-2632
+
+  if (const auto *RecordDecl = CanonicalType->getAsCXXRecordDecl()) {
+    if (!HasNonDeletedDefaultedEqualityComparison(CanonicalType->getAsCXXRecordDecl()))
+      return false;
----------------
Avoids calling the same method twice and avoids using a type name as a declaration identifier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147175