[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/Type.h:811
 
+  bool hasTrivialABIOverride() const;
+
----------------
This should get a comment, even if it's just to refer to the CXXRecordDecl method.


================
Comment at: lib/CodeGen/CGCall.cpp:3498
   bool HasAggregateEvalKind = hasAggregateEvaluationKind(type);
+  bool HasTrivialABIOverride = type.hasTrivialABIOverride();
 
----------------
Please sink this call to the points where you use it; it should be possible to avoid computing it in most cases.


================
Comment at: lib/CodeGen/CGCall.cpp:3505
+       CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) ||
+      HasTrivialABIOverride) {
     // If we're using inalloca, use the argument memory.  Otherwise, use a
----------------
e.g. here you can conditionalize it on HasAggregateEvalKind, which is both slightly faster and clearer to the reader.


================
Comment at: lib/CodeGen/CGCall.cpp:3518
+         CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+        HasTrivialABIOverride;
     if (DestroyedInCallee)
----------------
And here you can guard it by RD && RD->hasNonTrivialDestructor(), and you can just call hasNonTrivialABIOverride() directly on RD.


================
Comment at: lib/Sema/SemaDecl.cpp:11700
+    }
+  }
+
----------------
I think it's correct not to call CheckDestructorAccess and DiagnoseUseOfDecl here, since according to the standard destructor access is always supposed to be checked at the call-site, but please leave a comment explaining that.


https://reviews.llvm.org/D41039