[PATCH] D18139: [Cxx1z] Implement Lambda Capture of *this by Value as [=, *this] (P0018R3)

[Faisal Vali via cfe-commits]

faisalv marked 14 inline comments as done.
faisalv added a comment.

Thanks for the review! An updated patch that should address your concerns will follow in a few mins.

I'm still not entirely sure I follow your comment about the intialized-entity: 'directly pass the fact that the capture in question is a capture of *this'.


================
Comment at: lib/Sema/SemaExprCXX.cpp:841-849
@@ -840,11 +840,11 @@
   }
   if (ThisTy.isNull()) {
     if (isGenericLambdaCallOperatorSpecialization(CurContext) &&
         CurContext->getParent()->getParent()->isRecord()) {
       // This is a generic lambda call operator that is being instantiated
       // within a default initializer - so use the enclosing class as 'this'.
       // There is no enclosing member function to retrieve the 'this' pointer
       // from.
       QualType ClassTy = Context.getTypeDeclType(
           cast<CXXRecordDecl>(CurContext->getParent()->getParent()));
       // There are no cv-qualifiers for 'this' within default initializers, 
----------------
rsmith wrote:
> (Only somewhat related to the current change...) This looks wrong. If we're in a lambda within a lambda within a default member initializer, we need to recurse up more parents to find the right context. Looks like we should be walking up to the parent of the closure type, checking whether that is itself a lambda, and if so, recursing, until we reach a class or a function that isn't a lambda call operator. And we should accumulate the constness of `*this` on the way.
Thanks for taking a look at this.  Agreed - I'll submit this as a separate fix - added a fixme.

================
Comment at: lib/Sema/SemaExprCXX.cpp:919
@@ +918,3 @@
+    InitializedEntity Entity = InitializedEntity::InitializeLambdaCapture(
+      &S.Context.Idents.get("*this"), CaptureThisTy, Loc);
+    InitializationKind InitKind = InitializationKind::CreateDirect(Loc, Loc, Loc);
----------------
rsmith wrote:
> Please don't invent a fake identifier `*this` here. Instead, directly pass the fact that the capture in question is a capture of `*this`.
OK - I don't invent a fake identifier and instead pass in nullptr - but to be honest, I'm not sure what you mean by 'directly pass the fact that the capture in question is a capture of *this'

================
Comment at: lib/Sema/SemaExprCXX.cpp:982
@@ -950,3 +981,3 @@
       // For lambda expressions, build a field and an initializing expression.
-      ThisExpr = captureThis(Context, LSI->Lambda, ThisTy, Loc);
+      ThisExpr = captureThis(*this, Context, LSI->Lambda, ThisTy, Loc, ByCopy);
     else if (CapturedRegionScopeInfo *RSI
----------------
rsmith wrote:
> This looks wrong: for all scopes other than the innermost one, if `*this` is not already captured (explicitly) then it should always be captured by reference.
You're absolutely right.  Fixed. [=] { return [*this] { return m; }; }; will now emit the right code.


http://reviews.llvm.org/D18139