rnk added a comment.
Thank for the update, apologies for not providing these suggestions the first
time.
================
Comment at: clang/lib/CodeGen/CGCXXABI.cpp:320-321
+ // No virtual functions
+ // Additionally, we need to ensure that there is a trivial copy assignment
+ // operator, a trivial destructor and no user-provided constructors.
+ if (RD->hasProtectedFields() || RD->hasPrivateFields())
----------------
rnk wrote:
> I realize now that the name I chose (`ixCXX14Aggregate`) isn't very good
> because this routine also checks for trivial copy assignment and trivial
> destruction.
We still need a better name for this. Something like,
`isTrivialForAArch64MSVC`. This isn't a test for C++14 aggregate-ness, it's the
test that a specific compiler uses to decide if a record may be passed in
registers.
---
After thinking about this more, making this a virtual CGCXXABI method, as I
suggest below, makes more sense, because then the CGCXXABI class doesn't need
to expose this very MSVC-specific method, and we don't need to hoist this code
out of MicrosoftCXXABI.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5123
// If this is a C++ record, check the bases first.
if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
----------------
This comment is stale. We're looking for something more along the lines of,
"... check the C++ properties of the record, such as base classes."
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5137
+
+ if (!isHomogeneousAggregateForABI(CXXRD))
+ return false;
----------------
Apologies for moving the goalposts, but after re-reading the code, I feel like
it would make more sense to make this a CGCXXABI virtual method. This code then
would read:
if (!getCXXABI().isPermittedToBeHomogeneousAggregate(CXXRD))
return false;
IMO it also makes sense to move this up before checking bases.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92751/new/
https://reviews.llvm.org/D92751
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
