ahatanak marked 4 inline comments as done.
ahatanak added a comment.
In https://reviews.llvm.org/D41039#969171, @rjmccall wrote:
> I'll trust Richard on the tricky Sema/AST bits. The functionality of the
> patch looks basically acceptable to me, although I'm still not thrilled about
> the idea of actually removing the attribute from the AST rather than just
> letting it not have effect. But we could clean that up later if it's
> significantly simpler to do it this way.
Sure, we can clean up this later.
> Please add a CodeGenObjCXX test case that `__weak` fields in ARC++ do
> actually override the `trivial_abi` attribute but that `__strong` fields do
> not. Also, your test case does not seem to actually test arrays of `__weak`
> references.
Done. When I was writing a test that tests `__strong` fields, I found out that
the destructor of the struct wasn't being declared in the AST, which caused an
assertion to fail in CodeGenFunction::destroyCXXObject when compiling a
function that takes a trivial_abi type. I fixed it by forcing the declaration
of the destructor in Sema::ActOnParamDeclarator.
================
Comment at: include/clang/Basic/Attr.td:1159
+def TrivialABI : InheritableAttr {
+ let Spellings = [Clang<"trivial_abi">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
aaron.ballman wrote:
> Would this attribute make sense in C, or does it really only make sense in
> C++? If it doesn't make sense in C++, then you should also set `LangOpts` to
> be `CPlusPlus`. If it does make sense in C, then the Clang spelling should be
> `Clang<"trivial_abi", 1>`
I don't think this attribute makes sense in C, so I've set the LangOpts to be
CPlusPlus.
================
Comment at: lib/CodeGen/CGDecl.cpp:1825
+ HasTrivialABIOverride =
+ RD->canPassInRegisters() && RD->hasNonTrivialDestructor();
+
----------------
rjmccall wrote:
> You could make a helper function to do this computation and then declare it
> in some internal-to-IRGen header so that you don't write it out multiple
> times.
It turns out Sema needs this function too. I defined function
hasTrivialABIOverride in CXXRecordDecl and moved the code there.
https://reviews.llvm.org/D41039
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
