[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

John McCall rjmccall at apple.com
Sun May 1 11:21:36 PDT 2011


On May 1, 2011, at 9:00 AM, Sean Hunt wrote:
> 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.

The convention is that methods which access data which is only valid in
certain configurations of the AST node they're on should be named to
make that clear, and unless there's a good reason not to, they should
assert on that to catch bugs.  Obviously those aren't universally followed,
but more consistent is better.

>>> + // 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.

It's a code-size / compile-time optimization, mostly.  It's also already
suppressed if there are any virtual bases.  Please go ahead and enable
this.

>>> +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.

Make sure you call the appropriate destructor and that you
do it non-virtually.

> 
>>> 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.

Ah, good point.

>> 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.

You don't need to give multiple errors, but you do need the compiler
not to hang indefinitely because (1) it keeps finding Targets but
(2) none of them are Constructor.  My example currently hangs
the compiler.  Please fix this.

Joh.



More information about the cfe-commits mailing list