[PATCH] D147175: [clang] Add __is_trivially_equality_comparable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.)



================
Comment at: clang/include/clang/AST/DeclCXX.h:1449
   /// Notify the class that this destructor is now selected.
-  /// 
+  ///
   /// Important properties of the class depend on destructor properties. Since
----------------
Spurious whitespace change.


================
Comment at: clang/lib/AST/Type.cpp:2598-2599
+static bool HasDefaultedEqualityComparison(const CXXRecordDecl *Decl) {
+  if (Decl->isUnion())
+    return false;
+
----------------
Hmmm, is this correct? I thought there was a defaulted equality comparison operator in this case, but it's defined as deleted.

http://eel.is/c++draft/class.compare.default#2

Perhaps this function is looking for a usable defaulted equality comparison operator and not just "does it have one at all"?


================
Comment at: clang/lib/AST/Type.cpp:2616-2617
+                      }) &&
+         llvm::all_of(Decl->fields(), [](const FieldDecl *FD) {
+           if (!FD->getType()->isRecordType())
+             return true;
----------------
Do we have to look for fields with references per http://eel.is/c++draft/class.compare.default#2 ?


================
Comment at: clang/lib/AST/Type.cpp:4643-4645
+                        QualType Deduced, AutoTypeKeyword Keyword,
+                        bool IsDependent, ConceptDecl *CD,
+                        ArrayRef<TemplateArgument> Arguments) {
----------------
Spurious whitespace changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147175