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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
