[cfe-commits] r130642 - in /cfe/trunk: include/clang/AST/DeclCXX.h include/clang/AST/ExprCXX.h include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Initialization.h include/clang/Sema/Sema.h lib/CodeGen/CGClass.cpp lib/CodeGen/CGExprCXX.cpp lib/CodeGen/CodeGenFunction.h lib/Lex/PPMacroExpansion.cpp lib/Sema/SemaDeclCXX.cpp lib/Sema/SemaInit.cpp test/CodeGenCXX/cxx0x-delegating-ctors.cpp test/SemaCXX/cxx0x-delegating-ctors.cpp www/cxx_status.html

[Sean Hunt]

On 11-05-01 04:14 AM, John McCall wrote:
> On May 1, 2011, at 12:04 AM, Sean Hunt wrote:
>> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=130642&r1=130641&r2=130642&view=diff
>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=130642&r1=130641&r2=130642&view=diff>
>> ==============================================================================
>> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclCXX.h Sun May 1 02:04:31 2011
>> @@ -1608,6 +1608,23 @@
>> void setCtorInitializers(CXXCtorInitializer ** initializers) {
>> CtorInitializers = initializers;
>> }
>> +
>> + /// isDelegatingConstructor - Whether this constructor is a
>> + /// delegating constructor
>> + bool isDelegatingConstructor() const {
>> + return (getNumCtorInitializers() == 1) &&
>> + CtorInitializers[0]->isDelegatingInitializer();
>> + }
>> +
>> + /// getTargetConstructor - When this constructor delegates to
>> + /// another, retrieve the target
>> + CXXConstructorDecl *getTargetConstructor() const {
>> + if (isDelegatingConstructor())
>> + return CtorInitializers[0]->getTargetConstructor();
>> + else
>> + return 0;
>> + }
>> +
>
> My preference would be for this method to assert(isDelegatingConstructor()).
> Also, I think the name ought to have 'delegate' in there somewhere, maybe
> 'getDelegateTargetConstructor()'.

Okay. I wasn't really sure on the conventions, and this variant made its 
uses quicker to write. I don't have strong preferences.

>> Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=130642&r1=130641&r2=130642&view=diff
>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=130642&r1=130641&r2=130642&view=diff>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGClass.cpp Sun May 1 02:04:31 2011
>> @@ -658,6 +658,10 @@
>> if (Ctor->getType()->getAs<FunctionProtoType>()->isVariadic())
>> return false;
>>
>> + // FIXME: Decide if we can do a delegation of a delegating constructor.
>> + if (Ctor->isDelegatingConstructor())
>> + return false;
>> +
>
> Complete-to-base delegation is still fine for delegating constructors if
> there are no virtual bases, because by definition the complete and base
> variants of the target constructor are equivalent.

I think so, but I'd rather not accidentally break something and have to 
fix it later. Moreover, applying this optimization won't lead to a lot 
of direct speedup as we'll still have three constructors total being 
called. Writing a version of the optimization that causes the delegating 
complete constructor to do the VTT setup (which it normally doesn't as 
the target constructor does that) and then call into the base target 
constructor is probably ideal.

>> @@ -721,8 +728,10 @@
>>
>> if (Member->isBaseInitializer())
>> EmitBaseInitializer(*this, ClassDecl, Member, CtorType);
>> - else
>> + else if (Member->isAnyMemberInitializer())
>> MemberInitializers.push_back(Member);
>> + else
>> + llvm_unreachable("Delegating initializer on non-delegating
>> constructor");
>> }
>
> Better to just assert(Member->isAnyMemberInitializer());

Good catch.

>> +void
>> +CodeGenFunction::EmitDelegatingCXXConstructorCall(const
>> CXXConstructorDecl *Ctor,
>> + const FunctionArgList &Args) {
>> + assert(Ctor->isDelegatingConstructor());
>> +
>> + llvm::Value *ThisPtr = LoadCXXThis();
>> +
>> + AggValueSlot AggSlot = AggValueSlot::forAddr(ThisPtr, false,
>> /*Lifetime*/ true);
>> +
>> + EmitAggExpr(Ctor->init_begin()[0]->getInit(), AggSlot);
>> +}
>
> This is clever. After the delegate call succeeds, though, you do
> need to push a cleanup to call the dtor if an exception is thrown
> in the body of the delegating constructor.

Thanks! I thought I had everything but you're right about this.

>> Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=130642&r1=130641&r2=130642&view=diff
>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=130642&r1=130641&r2=130642&view=diff>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Sun May 1 02:04:31 2011
>> @@ -404,9 +404,16 @@
>> E->arg_begin(), E->arg_end());
>> }
>> else {
>> - CXXCtorType Type =
>> - (E->getConstructionKind() == CXXConstructExpr::CK_Complete)
>> - ? Ctor_Complete : Ctor_Base;
>> + CXXCtorType Type;
>> + CXXConstructExpr::ConstructionKind K = E->getConstructionKind();
>> + if (K == CXXConstructExpr::CK_Delegating) {
>> + // We should be emitting a constructor; GlobalDecl will assert this
>> + Type = CurGD.getCtorType();
>> + } else {
>> + Type = (E->getConstructionKind() == CXXConstructExpr::CK_Complete)
>> + ? Ctor_Complete : Ctor_Base;
>> + }
>
> Since this is already inside an if/else, go ahead and break up this
> ternary operator, or even consider using a switch over the construction
> kind.

Okay.

>> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=130642&r1=130641&r2=130642&view=diff
>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=130642&r1=130641&r2=130642&view=diff>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Sun May 1 02:04:31 2011
>> @@ -2068,6 +2068,30 @@
>>
>> return false;
>> }
>> +
>> +bool
>> +Sema::SetDelegatingInitializer(CXXConstructorDecl *Constructor,
>> + CXXCtorInitializer *Initializer) {
>> + Constructor->setNumCtorInitializers(1);
>> + CXXCtorInitializer **initializer =
>> + new (Context) CXXCtorInitializer*[1];
>> + memcpy(initializer, &Initializer, sizeof (CXXCtorInitializer*));
>> + Constructor->setCtorInitializers(initializer);
>> +
>> + // FIXME: This doesn't catch indirect loops yet
>> + CXXConstructorDecl *Target = Initializer->getTargetConstructor();
>> + while (Target) {
>> + if (Target == Constructor) {
>> + Diag(Initializer->getSourceLocation(), diag::err_delegating_ctor_loop)
>> + << Constructor;
>> + return true;
>> + }
>> + Target = Target->getTargetConstructor();
>> + }
>> +
>> + return false;
>> +}
>
> Is this FIXME still correct? It certainly looks like it detects loops of
> arbitrary length.

Yes, it is. In an indirect loop, the issue of multiple Decls for the 
same object comes into play. Pretty easy fix, but it was late and is 
just QOI.

> However, it also looks like it can infinite-loop if there's a loop not
> involving the current constructor, e.g.
> MyClass() : MyClass(0) {}
> MyClass(int x) : MyClass() { this->x = x; } // should get an error here,
> but then...
> MyClass(int x, int y) : MyClass(x) { this->y = y; } // this will
> infinite loop.
>
> John.

I'm not too concerned about this case, since when the second one errors 
and the user fixes that, then we're good. Doesn't seem very good to give 
errors multiple times.

Sean