<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On May 1, 2011, at 12:04 AM, Sean Hunt wrote:</div><div><blockquote type="cite"><div>Modified: cfe/trunk/include/clang/AST/DeclCXX.h<br>URL: <a href="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</a><br>==============================================================================<br>--- cfe/trunk/include/clang/AST/DeclCXX.h (original)<br>+++ cfe/trunk/include/clang/AST/DeclCXX.h Sun May 1 02:04:31 2011<br>@@ -1608,6 +1608,23 @@<br> void setCtorInitializers(CXXCtorInitializer ** initializers) {<br> CtorInitializers = initializers;<br> }<br>+<br>+ /// isDelegatingConstructor - Whether this constructor is a<br>+ /// delegating constructor<br>+ bool isDelegatingConstructor() const {<br>+ return (getNumCtorInitializers() == 1) &&<br>+ CtorInitializers[0]->isDelegatingInitializer();<br>+ }<br>+<br>+ /// getTargetConstructor - When this constructor delegates to<br>+ /// another, retrieve the target<br>+ CXXConstructorDecl *getTargetConstructor() const {<br>+ if (isDelegatingConstructor())<br>+ return CtorInitializers[0]->getTargetConstructor();<br>+ else<br>+ return 0;<br>+ }<br>+<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div><div>My preference would be for this method to assert(isDelegatingConstructor()).</div><div>Also, I think the name ought to have 'delegate' in there somewhere, maybe</div><div>'getDelegateTargetConstructor()'.</div></div><div><br></div><blockquote type="cite"><div>Modified: cfe/trunk/lib/CodeGen/CGClass.cpp<br>URL: <a href="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</a><br>==============================================================================<br>--- cfe/trunk/lib/CodeGen/CGClass.cpp (original)<br>+++ cfe/trunk/lib/CodeGen/CGClass.cpp Sun May 1 02:04:31 2011<br>@@ -658,6 +658,10 @@<br> if (Ctor->getType()->getAs<FunctionProtoType>()->isVariadic())<br> return false;<br><br>+ // FIXME: Decide if we can do a delegation of a delegating constructor.<br>+ if (Ctor->isDelegatingConstructor())<br>+ return false;<br>+<br></div></blockquote><div><br></div><div>Complete-to-base delegation is still fine for delegating constructors if</div><div>there are no virtual bases, because by definition the complete and base</div><div>variants of the target constructor are equivalent.</div><div><br></div></div><div><blockquote type="cite"><div>@@ -721,8 +728,10 @@</div><div><br> if (Member->isBaseInitializer())<br> EmitBaseInitializer(*this, ClassDecl, Member, CtorType);<br>- else<br>+ else if (Member->isAnyMemberInitializer())<br> MemberInitializers.push_back(Member);<br>+ else<br>+ llvm_unreachable("Delegating initializer on non-delegating constructor");<br> }<br></div></blockquote><div><br></div>Better to just assert(Member->isAnyMemberInitializer());</div><div><br></div><div><blockquote type="cite"><div>+void<br>+CodeGenFunction::EmitDelegatingCXXConstructorCall(const CXXConstructorDecl *Ctor,<br>+ const FunctionArgList &Args) {<br>+ assert(Ctor->isDelegatingConstructor());<br>+<br>+ llvm::Value *ThisPtr = LoadCXXThis();<br>+<br>+ AggValueSlot AggSlot = AggValueSlot::forAddr(ThisPtr, false, /*Lifetime*/ true);<br>+<br>+ EmitAggExpr(Ctor->init_begin()[0]->getInit(), AggSlot);<br>+}<br></div></blockquote><br></div><div>This is clever. After the delegate call succeeds, though, you do</div><div>need to push a cleanup to call the dtor if an exception is thrown</div><div>in the body of the delegating constructor.</div><div><br></div><div><blockquote type="cite"><div>Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp<br>URL: <a href="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</a><br>==============================================================================<br>--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)<br>+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Sun May 1 02:04:31 2011<br>@@ -404,9 +404,16 @@<br> E->arg_begin(), E->arg_end());<br> }<br> else {<br>- CXXCtorType Type = <br>- (E->getConstructionKind() == CXXConstructExpr::CK_Complete) <br>- ? Ctor_Complete : Ctor_Base;<br>+ CXXCtorType Type;<br>+ CXXConstructExpr::ConstructionKind K = E->getConstructionKind();<br>+ if (K == CXXConstructExpr::CK_Delegating) {<br>+ // We should be emitting a constructor; GlobalDecl will assert this<br>+ Type = CurGD.getCtorType();<br>+ } else {<br>+ Type = (E->getConstructionKind() == CXXConstructExpr::CK_Complete)<br>+ ? Ctor_Complete : Ctor_Base;<br>+ }<br></div></blockquote><div><br></div><div>Since this is already inside an if/else, go ahead and break up this</div><div>ternary operator, or even consider using a switch over the construction kind.</div><div><br></div><blockquote type="cite"><div>Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp</div><div>URL: <a href="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</a><br>==============================================================================<br>--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)<br>+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Sun May 1 02:04:31 2011<br>@@ -2068,6 +2068,30 @@<br><br> return false;<br>}<br>+<br>+bool<br>+Sema::SetDelegatingInitializer(CXXConstructorDecl *Constructor,<br>+ CXXCtorInitializer *Initializer) {<br>+ Constructor->setNumCtorInitializers(1);<br>+ CXXCtorInitializer **initializer =<br>+ new (Context) CXXCtorInitializer*[1];<br>+ memcpy(initializer, &Initializer, sizeof (CXXCtorInitializer*));<br>+ Constructor->setCtorInitializers(initializer);<br>+<br>+ // FIXME: This doesn't catch indirect loops yet</div></blockquote><div><blockquote type="cite"><div>+ CXXConstructorDecl *Target = Initializer->getTargetConstructor();<br>+ while (Target) {<br>+ if (Target == Constructor) {<br>+ Diag(Initializer->getSourceLocation(), diag::err_delegating_ctor_loop)<br>+ << Constructor;<br>+ return true;<br>+ }<br>+ Target = Target->getTargetConstructor();<br>+ }<br>+<br>+ return false;<br>+}<br></div></blockquote><br></div>Is this FIXME still correct? It certainly looks like it detects loops of</div><div>arbitrary length.</div><div><br></div><div>However, it also looks like it can infinite-loop if there's a loop not</div><div>involving the current constructor, e.g.</div><div> MyClass() : MyClass(0) {}</div><div> MyClass(int x) : MyClass() { this->x = x; } // should get an error here, but then...</div><div> MyClass(int x, int y) : MyClass(x) { this->y = y; } // this will infinite loop.</div><div><br></div><div>John.</div></body></html>