ahatanak marked 3 inline comments as done.
ahatanak added inline comments.
================
Comment at: include/clang/AST/DeclCXX.h:1489-1491
+ bool shouldBeDestructedInCallee() const {
+ return data().CanPassInRegisters && hasNonTrivialDestructor();
+ }
----------------
rsmith wrote:
> This will return incorrect results on MSVC, where every class type should be
> destroyed in the callee. Since this is only used by CodeGen, and only used
> in combination with the MSVC "destroy right-to-left in callee" ABI setting,
> please sink it to CodeGen.
I sank the code to CGCall.cpp and CGDecl.cpp and removed the method.
================
Comment at: lib/CodeGen/CGCall.cpp:3518
+ CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+ TypeDestructedInCallee;
if (DestroyedInCallee)
----------------
rsmith wrote:
> This looks wrong (but the wrongness is pre-existing): in the weird MSVC
> x86_64 case where a small object of class type is passed in registers despite
> having a non-trivial destructor, this will set `DestroyedInCallee` to false,
> meaning that the parameter will be destroyed twice, in both the caller and
> callee.
>
> And yep, that's exactly what both Clang and MSVC do:
> https://godbolt.org/g/zjeQq6
>
> If we fixed that, we could unconditionally `setExternallyDestructed()` here.
> But let's leave that discussion for a separate patch :)
OK, I'll leave this for future work.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:5923
+ M->setTrivialForCall(HasTrivialABI);
+ Record->finishedUserProvidedMethod(M);
+ }
----------------
rsmith wrote:
> This should have a different name if you're only going to call it for
> user-provided copy ctors, move ctors, and dtors.
I renamed the function to setTrivialForCallFlags.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:12108-12114
+ if (!IsTrivialForCall) {
+ if (ClassDecl->needsOverloadResolutionForCopyConstructor())
+ IsTrivialForCall =
+ SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor, true);
+ else
+ IsTrivialForCall = ClassDecl->hasTrivialCopyConstructorForCall();
+ }
----------------
rsmith wrote:
> Nit: can you use the same form of `?:` expression as is used a few lines
> above to make it obvious to a reader that this computation parallels that one?
Done. I clang-formated the code above too.
https://reviews.llvm.org/D41039
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
