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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/LambdaCapture.h:41
@@ -41,3 +40,3 @@
   SourceLocation EllipsisLoc;
-
+  bool IsStarThis : 1;
   friend class ASTStmtReader;
----------------
I'm not a fan of this representation. Please use `Capture_ByCopy` to indicate a by-copy capture of `*this`. It's probably time to clean up `DeclAndBits` a little. Something like this should work:

  struct LLVM_ALIGNAS(4) OpaqueCapturedEntity {};
  llvm::PointerIntPair<void*, 2> CapturedEntityAndBits;
  static OpaqueCapturedEntity StarThis;
  static OpaqueCapturedEntity VLAType;

where the `void*` is either a `Decl*` or points to `StarThis` or `VLAType`.

================
Comment at: include/clang/Basic/Lambda.h:35
@@ -34,2 +34,3 @@
 enum LambdaCaptureKind {
   LCK_This,   ///< Capturing the \c this pointer
+  LCK_StarThis, /// < Capturing the \c *this object by value
----------------
-> Capturing the \c *this object by reference?

================
Comment at: include/clang/Sema/ScopeInfo.h:414-417
@@ -413,6 +413,6 @@
     // variables are captured by copy.
     enum CaptureKind {
       Cap_ByCopy, Cap_ByRef, Cap_Block, Cap_This
     };
-
+    bool IsStarThis = false;
     /// The variable being captured (if we are not capturing 'this') and whether
----------------
Please try to integrate this into the existing code rather than adding it as a separate thing on the side. Again, we can model `this` and `*this` as capturing the same entity either by reference or by value, and we should do so.

================
Comment at: include/clang/Sema/ScopeInfo.h:455-468
@@ -453,12 +454,16 @@
 
     bool isThisCapture() const {
       return InitExprAndCaptureKind.getInt() == Cap_This;
     }
+    bool isStarThisCapture() const {
+      return IsStarThis;
+    }
     bool isVariableCapture() const {
-      return InitExprAndCaptureKind.getInt() != Cap_This && !isVLATypeCapture();
+      return !isThisCapture() && !isVLATypeCapture();
     }
     bool isCopyCapture() const {
-      return InitExprAndCaptureKind.getInt() == Cap_ByCopy &&
-             !isVLATypeCapture();
+      return (InitExprAndCaptureKind.getInt() == Cap_ByCopy &&
+              !isVLATypeCapture()) ||
+             IsStarThis;
     }
     bool isReferenceCapture() const {
----------------
In particular: don't add `isStarThisCapture`, just use `isThisCapture` and `isCopyCapture`/`isReferenceCapture` to distinguish between `this` and `*this`.

================
Comment at: lib/AST/ExprCXX.cpp:894
@@ -892,2 +893,3 @@
+    IsStarThis = true;
   case LCK_This:
     assert(!Var && "'this' capture cannot have a variable!");
----------------
Add // Fall through comment.

================
Comment at: lib/Parse/ParseExprCXX.cpp:850
@@ +849,3 @@
+
+    if (Tok.is(tok::star) && NextToken().is(tok::kw_this)) {
+      Kind = LCK_StarThis;
----------------
You don't need lookahead for this; if the current token is `*`, you can consume that token then look for `this`. It's an error if something else comes next.

================
Comment at: lib/Parse/ParseExprCXX.cpp:857-860
@@ +856,6 @@
+      ConsumeToken();
+      Diag(Loc, !getLangOpts().CPlusPlus1z
+                             ? diag::ext_star_this_lambda_capture_cxx1z
+                             : diag::warn_cxx14_compat_star_this_lambda_capture);
+
+    } else if (Tok.is(tok::kw_this)) {
----------------
We shouldn't produce this warning during disambiguation between a lambda and an Obj-C message send or designator. Please move the diagnostic into Sema instead.

================
Comment at: lib/Sema/SemaDecl.cpp:10994
@@ -10994,1 +10993,3 @@
+                              S.getCurrentThisType(), /*Expr*/ nullptr,
+                              !I->getType()->isPointerType());
     } else {
----------------
Use `C.getCaptureKind()` rather than querying the type of the field directly here.

================
Comment at: lib/Sema/SemaExpr.cpp:13256
@@ -13255,3 +13255,3 @@
 /// being captured.
-static void addAsFieldToClosureType(Sema &S, LambdaScopeInfo *LSI, VarDecl *Var,
+static void addAsFieldToClosureType(Sema &S, LambdaScopeInfo *LSI, VarDecl *,
                                     QualType FieldType, QualType DeclRefType,
----------------
Just delete this parameter? (As a separate change, though.)

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

================
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);
----------------
Please don't invent a fake identifier `*this` here. Instead, directly pass the fact that the capture in question is a capture of `*this`.

================
Comment at: lib/Sema/SemaExprCXX.cpp:929-932
@@ -898,5 +928,6 @@
 
 bool Sema::CheckCXXThisCapture(SourceLocation Loc, bool Explicit, 
-    bool BuildAndDiagnose, const unsigned *const FunctionScopeIndexToStopAt) {
+    bool BuildAndDiagnose, const unsigned *const FunctionScopeIndexToStopAt,
+    const bool ByCopy) {
   // We don't need to capture this in an unevaluated context.
   if (isUnevaluatedContext() && !Explicit)
----------------
Please `assert(!ByCopy || Explicit)` here (we cannot implicitly capture `*this` by value).

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

================
Comment at: lib/Sema/SemaLambda.cpp:938
@@ -937,3 +937,3 @@
       }
-
+      // FIXME: Fix this comment from the wording for lambda '*this' wording.
       // C++11 [expr.prim.lambda]p8:
----------------
Just make this change?


http://reviews.llvm.org/D18139