[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]

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
> ==============================================================================
> --- 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()'.

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

> @@ -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());

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

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

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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110501/89f9f7ed/attachment.html>