[PATCH] Implement capturing for generic lambdas

[Faisal Vali]

================
Comment at: lib/Sema/SemaExprCXX.cpp:5788
@@ +5787,3 @@
+  const bool IsFullExprInstantiationDependent = 
+      FE->isInstantiationDependent() || FE->getType()->isDependentType();
+  // All the potentially captureable variables in the current nested 
----------------
Richard Smith wrote:
> Use `FE->isTypeDependent()`, not `FE->getType()->isDependentType()`. The latter can be `false` when the former is `true` in cases where we know what the type is but the standard says we're not supposed to.
I think I can just get rid of the typedependence check - since i do an instantiation dependence check

================
Comment at: include/clang/AST/ASTLambda.h:68-77
@@ -67,2 +67,12 @@
 
+
+// This returns the parent DeclContext ensuring that the correct
+// parent DeclContext is returned for Lambdas
+inline DeclContext *getLambdaAwareParentOfDeclContext(DeclContext *DC) {
+  if (isLambdaCallOperator(DC))
+    return DC->getParent()->getParent();
+  else 
+    return DC->getParent();
+}
+
 } // clang
----------------
Richard Smith wrote:
> Please merge this with SemaExpr's `getParentOfCapturingContextOrNull`.
OK - will have getParentOfCapturingContextOrNull - call this function.

================
Comment at: lib/Sema/SemaExpr.cpp:11836
@@ -11824,1 +11835,3 @@
+  }
+  DeclContext *const LLVM_ATTRIBUTE_UNUSED StartingDeclContext = DC;
 
----------------
Richard Smith wrote:
> We'd usually write `(void)StartingDeclContext;` rather than `LLVM_ATTRIBUTE_UNUSED`.
OK - but i'll get rid of this for now, since it relates to a better diagnostic below, which you recommend I submit as a separate patch.

================
Comment at: lib/Sema/SemaExpr.cpp:11905-11911
@@ -11874,1 +11904,9 @@
              diag::note_lambda_decl);
+        // FVQUESTION: If we error out because an outer lambda can not implicitly
+        // capture a variable that an inner lambda explicitly captures, we
+        // should have the inner lambda do the explicit capture - because
+        // it makes for cleaner diagnostics later.  Any thoughts on the merits
+        // of this? This is purely done just so the diagnostic does not 
+        // misleadingly claim that a variable can not be captured by a lambda 
+        // implicitly even though it is captured explicitly.
+        if (VariableCaptureWasInitiallyExplicit) {
----------------
Richard Smith wrote:
> This sounds like a good idea, but should probably be done in a separate patch. Do generic lambda captures rely on this somehow?
OK - will expunge for now.

================
Comment at: lib/Sema/SemaExpr.cpp:12016-12019
@@ +12015,6 @@
+  if (LambdaScopeInfo *LSI = getCurLambda()) {
+    for (int I = LSI->getNumPotentialCaptures(); I--; ) {
+      Expr *ExprVarUsedIn = 0; 
+      VarDecl *Var = 0;
+      LSI->getPotentialCapture(I, Var, ExprVarUsedIn);
+      if (Var && E->IgnoreParens() == ExprVarUsedIn) {
----------------
Richard Smith wrote:
> Looks like an off-by-one error here? Should `I` start at `NumPotentialCaptures - 1`?
It shouldn't be off by one - we start at NumPotentialCaptures, then decrement I right away and if we start at 1 or 0,  the body should never be entered (postincrement).  But I can use a different iteration scheme if you want.

================
Comment at: lib/Sema/SemaExpr.cpp:12021-12025
@@ +12020,7 @@
+      if (Var && E->IgnoreParens() == ExprVarUsedIn) {
+        // Erase it from Potential Capture only if it is potentially
+        // not odr-usable since we just did our l-value-rvalue
+        // TODO: this needs to be added to DiscardValueExpr too
+        if (IsVariableAConstantExpression(Var, Context))
+          LSI->removePotentialCapture(ExprVarUsedIn);
+      }
----------------
Richard Smith wrote:
> I thought the idea for this case was to wait until we got to a full-expression boundary, and then check whether the entire expression was instantiation-dependent, and capture only if it is. For instance:
> 
>   const int n = 0;
>   [&] (auto x) {
>     (void)+n + x;
>   };
> 
> would capture `n`, even though it's obviously not odr-used here, because the full-expression is instantiation-dependent. I think we want to flag the potential capture as "only capture if the full-expression is dependent", and then check that flag once we get to the end of the full-expression. Or am I misrecalling?
Yes - as usual - you are correct - per the letter of the draft-standard.  But doesn't the capture seem gratuitous (we seem to have gone to some lengths to avoid capturing those entities that are simply named if we can do so, so why stop here?)  If we can unambiguously conclude that a constant will not be odr-used by analyzing its subexpression (regardless of the instantiation dependence of the full expression), shouldn't we avoid capturing it?  The case seems like it could happen relatively frequently (see what C++ committee meetings have done to me - I find myself feeling a little too comfortable making this claim with absolutely no data - fine, i'll retract it ;)

Does UpdateLValueToRValue only get called if we can unambiguously conclude the conversion must happen regardless of the instantiation context? (i.e. there is no altering those semantics?)

I think we can address this in our revised wording?  What say you? Nevertheless, the following patch will implement what you ask for (i.e. the letter of the current draft).




   

================
Comment at: lib/Sema/SemaExprCXX.cpp:5793
@@ +5792,3 @@
+     
+  for (size_t I = 0, N = CurrentLSI->getNumPotentialCaptures(); I < N; ++I) {
+    Expr *ExprTheVarIsUsedIn = 0;
----------------
Richard Smith wrote:
> Use `!=`, not `<`, here. Using `<` makes me think that you might be incrementing `I` within the loop.
OK.

================
Comment at: lib/Sema/SemaExprCXX.cpp:5897-5900
@@ -5800,1 +5896,6 @@
+  
+  LambdaScopeInfo *const CurrentLSI = getCurLambda(); 
+  if (CurrentLSI && CurrentLSI->hasPotentialCaptures() && 
+      !FullExpr.isInvalid())
+    CheckLambdaCaptures(FE, CurrentLSI, *this);
   return MaybeCreateExprWithCleanups(FullExpr);
----------------
Richard Smith wrote:
> Hmm, what happens for something like:
> 
>   void f() {
>     const int n = 0;
>     auto L =  [&](auto a) {
>       +n + ({ 0; a; });
>     };
>   }
> 
> Here, we see `+n`, and then the full-expression `0;` ends, so we don't capture `n` (and instead remove it from our list of potential captures), and then the full-expression `+n + ({ 0; });` ends, but it's too late for us to see that we need to capture `n` after all.
> 
> We can defer fixing this for now, because I think it only affects statement-expressions, but please add a FIXME.
> 
> [The same thing would go wrong if we parsed a lambda's init-captures before creating the new lambda scope, which seems like another argument for not doing init-captures that way.]
Will add a FIXME - although if I could convince you that it might be better to revise the standard-draft wording so that we don't need to capture 'n' in such a situation - that would be my preferred outcome (unless i'm missing some pathologic scenario).  



http://llvm-reviews.chandlerc.com/D2035