[cfe-commits] [PATCH] Implement C++11 in-class member initializers

[Douglas Gregor]

On Jun 11, 2011, at 10:26 AM, Richard Smith wrote:

> Hi!
> 
> Thanks for the review. Committed, along with the requested changes and
> some extra tests, in r132878.

Thanks!

> On Fri, June 10, 2011 19:06, Douglas Gregor wrote:
>> On May 23, 2011, at 5:48 PM, Richard Smith wrote:
>> +  if (FD->getType()->isDependentType() || InitExpr->isTypeDependent()) {
>> +    // We can't check dependent initializers until instantiation.
>> +
>> +    // Erase any temporaries within this evaluation context; we're not
>> +    // going to track them in the AST, since we'll be rebuilding the
>> +    // ASTs during template instantiation.
>> +    ExprTemporaries.erase(
>> +      ExprTemporaries.begin() + ExprEvalContexts.back().NumTemporaries,
>> +      ExprTemporaries.end());
>> 
>> 
>> Rather than directly poking at ExprTemporaries, please just move the
>> MaybeCreateExprWithCleanups() call (and subsequent Invalid check) out of
>> the next "else" block, so we perform the corresponding checking (e.g.,
>> for destructors) in all cases. Plus, we want as few places as possible to
>> modify ExprTemporaries.
> 
> Done. This approach was inspired by the corresponding code in
> Sema::BuildBaseInitializer and Sema::BuildMemberInitializer; should we
> apply a similar fix there too?

Yeah, that'd be a good idea.

>> -  // If we're not in an instance method, error out.
>> -  CXXMethodDecl *method = dyn_cast<CXXMethodDecl>(DC);
>> -  if (!method || !method->isInstance())
>> -    return 0;
>> +  QualType ThisTy;
>> +  if (CXXMethodDecl *method = dyn_cast<CXXMethodDecl>(DC)) {
>> +    if (method && method->isInstance())
>> +      ThisTy = method->getThisType(Context);
>> +  } else if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC)) {
>> +    // C++0x [expr.prim]p4:
>> +    //   Otherwise, if a member-declarator declares a non-static data
>> member +    // of a class X, the expression this is a prvalue of type
>> "pointer to X"
>> +    // within the optional brace-or-equal-initializer.
>> +    if (CurScope->getFlags() & Scope::ThisScope)
>> +      ThisTy = Context.getPointerType(Context.getRecordType(RD));
>> +  }
>> 
>> 
>> Please don't access CurScope directly... it's parser state that may not
>> represent the current state of Sema (e.g., because Sema could be in the
>> middle of instantiation). Instead, Sema routines driven by the parser
>> should pass a Scope* through to tryCaptureCXXThis(). TreeTransform, on
>> the other hand, will probably need to just instantiate the type of
>> CXXThisExpr rather than calling tryCaptureCXXThis(), since we won't have
>> a useful Scope*.
> 
> I've switched to using getScopeForContext; this also fixes an issue where
> 'this' was unavailable in scopes nested within in-class initializers (for
> instance, within blocks). I hope that's acceptable; if not, I'll fix with
> a subsequent commit.

Yeah, that's fine.

> This also exposed an issue in parsing: I was creating an unnecessary class
> scope nested within the actual class scope when parsing initializers, in
> order to update the flags to include ThisScope. I've modified the code to
> update the flags on the existing class scope.

Ah, much better.

>> // Mark that we're closing on 'this' in all the block scopes, if
>> applicable. -  for (unsigned idx = FunctionScopes.size() - 1;
>> -       isa<BlockScopeInfo>(FunctionScopes[idx]);
>> -       --idx)
>> -    cast<BlockScopeInfo>(FunctionScopes[idx])->CapturesCXXThis = true;
>> +  if (!ThisTy.isNull())
>> +    for (unsigned idx = FunctionScopes.size() - 1;
>> +         isa<BlockScopeInfo>(FunctionScopes[idx]);
>> +         --idx)
>> +      cast<BlockScopeInfo>(FunctionScopes[idx])->CapturesCXXThis = true;
>> 
>> 
>> We should only be doing this when 'this' is captured because we're in a
>> CXXMethodDecl. We might incorrectly think we're capturing the outer
>> 'this' in crazy code such as:
>> 
>> 
>> struct X { void f() { ^ {
>> struct Nested { Nested *ptr = this; }; }
>> };
>> };
> 
> I agree with your observation, but not the proposed fix: we want to mark
> 'this' as captured in cases such as this:
> 
> struct X {
>  X *(^f)() = ^{ return this; }
> };
> 
> I've fixed this by only marking 'this' as captured in the block scopes we
> skipped when scanning outwards for the containing method or class decl.

Yes, that's the right fix. Thanks!

> Testing for that exposed a couple of CodeGen issues with the interaction
> of 'this' in in-class member initializers and blocks, which I've fixed.
> The interesting such issue was that there is (apparently) no defined name
> mangling for blocks in in-class member initializers. Aside from an assert,
> the code was mangling the above block as if the struct were written as
> follows:
> 
> struct X {
>  X() : f(^{ return this; }) {}
>  X *(^f)();
> };
> 
> That doesn't seem particularly unreasonable to me, so I've just updated
> the assert.

That seems fine to me.

One step closer to '0x!

	- Doug