[PATCH] Implement capturing for generic lambdas

[Richard Smith]

On Wed, Nov 6, 2013 at 7:29 AM, Faisal Vali <faisalv at yahoo.com> wrote:

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


Ah, oops, clearly I didn't read this clearly enough. Using a more
conventional iteration scheme would make this more readable...


> ================
> 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?)
>

The problem is that the set of captures for a lambda is part of the ABI
(since lambda layout can be made visible through inline functions and the
like), and there are no guarantees as to which cases we'll manage to build
an lvalue-to-rvalue conversion in, when parsing a template -- some
seemingly harmless change elsewhere in Sema could cause us to start or stop
building such a node. So we need a rule that anyone can implement and get
exactly the same result.


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