[PATCH] Implement capturing for generic lambdas
Richard Smith
richard at metafoo.co.uk
Tue Nov 5 19:29:50 PST 2013
This is looking really good!
================
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
----------------
Please merge this with SemaExpr's `getParentOfCapturingContextOrNull`.
================
Comment at: include/clang/Sema/ScopeInfo.h:689
@@ +688,3 @@
+ bool hasPotentialThisCapture() const {
+ return PotentialThisCaptureLocation.isValid();
+ }
----------------
Indentation
================
Comment at: include/clang/Sema/ScopeInfo.h:715
@@ +714,3 @@
+ bool hasPotentialCaptures() const {
+ return getNumPotentialCaptures() ||
+ PotentialThisCaptureLocation.isValid();
----------------
Indentation
================
Comment at: lib/Sema/SemaDecl.cpp:9442
@@ +9441,3 @@
+ // Add the captures to the LSI so they can be noted as already
+ // captured within tryCaptureVar. (hence we don't need Expr* below)
+ for (LambdaExpr::capture_iterator C = LambdaClass->captures_begin(),
----------------
I don't think the parenthetical makes this clearer; drop.
================
Comment at: lib/Sema/SemaDecl.cpp:9444-9445
@@ +9443,4 @@
+ for (LambdaExpr::capture_iterator C = LambdaClass->captures_begin(),
+ CEnd = LambdaClass->captures_end();
+ C != CEnd; ++C) {
+ if (C->capturesVariable()) {
----------------
More indentation please =)
================
Comment at: lib/Sema/SemaExpr.cpp:11836
@@ -11824,1 +11835,3 @@
+ }
+ DeclContext *const LLVM_ATTRIBUTE_UNUSED StartingDeclContext = DC;
----------------
We'd usually write `(void)StartingDeclContext;` rather than `LLVM_ATTRIBUTE_UNUSED`.
================
Comment at: lib/Sema/SemaExpr.cpp:11886
@@ +11885,3 @@
+ } else
+ diagnoseUncapturableValueReference(*this, ExprLoc, Var, DC);
+ }
----------------
Indentation.
================
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) {
----------------
This sounds like a good idea, but should probably be done in a separate patch. Do generic lambda captures rely on this somehow?
================
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) {
----------------
Looks like an off-by-one error here? Should `I` start at `NumPotentialCaptures - 1`?
================
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);
+ }
----------------
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?
================
Comment at: lib/Sema/SemaExpr.cpp:12072-12074
@@ -12003,3 +12071,5 @@
- if (!IsPotentiallyEvaluatedContext(SemaRef))
- return;
+ // If the context is NOT a potentially evaluated context (thus is a dependent
+ // context) do not bother to consider variables in this context for odr-use
+ // unless we are within a lambda.
+ // We should be able to diagnose this error early even in a non-generic
----------------
A non-potentially-evaluated context need not be a dependent context.
================
Comment at: lib/Sema/SemaExpr.cpp:12081-12083
@@ -12006,1 +12080,5 @@
+ //
+ if (!IsPotentiallyEvaluatedContext(SemaRef)) {
+ if (SemaRef.isUnevaluatedContext()) return;
+
----------------
The effect of combining these checks is to check if we're in a "PotentiallyEvaluatedIfUsed" context, that is, if we're in a default argument. That doesn't seem to match the comments here. But the code here looks right: if we don't know whether the context is potentially evaluated or not (because we're in a default argument), we want to add a potential capture but not an odr-use.
================
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
----------------
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.
================
Comment at: lib/Sema/SemaExprCXX.cpp:5793
@@ +5792,3 @@
+
+ for (size_t I = 0, N = CurrentLSI->getNumPotentialCaptures(); I < N; ++I) {
+ Expr *ExprTheVarIsUsedIn = 0;
----------------
Use `!=`, not `<`, here. Using `<` makes me think that you might be incrementing `I` within the loop.
================
Comment at: lib/Sema/SemaExprCXX.cpp:5820
@@ +5819,3 @@
+ SourceLocation ExprLoc = ExprTheVarIsUsedIn->getExprLoc();
+ if(S.tryCaptureVariable(Var, ExprLoc, S.TryCapture_Implicit,
+ /*EllipsisLoc*/ SourceLocation(),
----------------
Space after `if`
================
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);
----------------
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.]
================
Comment at: lib/Sema/SemaExprMember.cpp:925
@@ +924,3 @@
+ // contains both static and non-static member functions).
+ if (!CheckCXXThisCapture(Loc, /*Explcit*/false, /*Diagnose*/false))
+ CheckCXXThisCapture(Loc);
----------------
Typo 'Explcit'
================
Comment at: lib/Sema/SemaExprCXX.cpp:5783-5784
@@ -5769,1 +5782,4 @@
+// If the variable's initializer is dependent, analyze it for any constructs
+// that we can use to conclude that the initializer can never be a constexpr.
+
----------------
This isn't quite correct: you need to avoid walking into subexpressions that might not be evaluated (operands of `?:`, RHS of `&&` and `||`, ....).
Please take this out of the patch for now, we can review this diagnostic/QoI improvement separately once we have the main functionality landed. (This seems like useful functionality for other diagnostics too.)
http://llvm-reviews.chandlerc.com/D2035
More information about the cfe-commits
mailing list