[PATCH] Generic Lambdas: The next step
Richard Smith
richard at metafoo.co.uk
Wed Sep 18 14:37:10 PDT 2013
This is very hard to review as one large monolithic change. Can you break it up into smaller pieces? For instance, a piece adding trivial Sema support for 'auto' parameters would be a good start (with no building of a call operator template or instantiation support or capture changes or anything like that).
================
Comment at: include/clang/AST/ASTContext.h:1133
@@ -1132,3 +1132,3 @@
QualType getAutoType(QualType DeducedType, bool IsDecltypeAuto,
- bool IsDependent = false) const;
+ bool IsDependent, bool IsParameterPack) const;
----------------
I'm not clear on why we need this extra flag. Given:
[](auto...x) {}
I'm imagining we'll generate
template<typename...T> auto operator()(U...x) {}
where `U` is an `AutoType` whose `DeducedType` is `T`. Thus we can say that `auto` contains an unexpanded pack if its deduced type does.
================
Comment at: include/clang/AST/ASTLambda.h:11
@@ +10,3 @@
+/// \file
+/// \brief Defines some utility functions on lambda related classes and functions.
+///
----------------
80 cols
================
Comment at: include/clang/AST/ASTLambda.h:22
@@ +21,3 @@
+
+namespace clang {
+static inline const char *getLambdaStaticInvokerName() {
----------------
Why are all the `inline` functions in this file declared `static`?
================
Comment at: include/clang/AST/ASTLambda.h:26-27
@@ +25,4 @@
+}
+// Retrieve the lambda's conversion operator if one exists.
+// For a generic lambda, return the templated conversion decl.
+static inline CXXConversionDecl *getLambdaConversionOperator(
----------------
///
================
Comment at: include/clang/AST/ASTLambda.h:28-48
@@ +27,23 @@
+// For a generic lambda, return the templated conversion decl.
+static inline CXXConversionDecl *getLambdaConversionOperator(
+ CXXRecordDecl *LambdaClass) {
+ CXXConversionDecl *Conv = 0;
+ if (!LambdaClass->isLambda()) return 0;
+
+ DeclContext *LambdaClassDC = cast<DeclContext>(LambdaClass);
+ for (DeclContext::decl_iterator M = LambdaClassDC->decls_begin();
+ M != LambdaClassDC->decls_end(); ++M ) {
+ CXXMethodDecl *MD = 0;
+ if (FunctionTemplateDecl *FTD = dyn_cast<FunctionTemplateDecl>(*M))
+ MD = cast<CXXMethodDecl>(FTD->getTemplatedDecl());
+ else
+ MD = cast<CXXMethodDecl>(*M);
+
+ if (CXXConversionDecl *C = dyn_cast_or_null<CXXConversionDecl>(MD)) {
+ assert(!Conv && "There should be only one conversion function!");
+ Conv = C;
+ }
+ }
+ return Conv;
+}
+static inline bool isLambdaConversionOperator(CXXConversionDecl *C) {
----------------
This looks too complex for a .h file. Move to .cpp?
================
Comment at: include/clang/AST/ASTLambda.h:56-58
@@ +55,5 @@
+ if (!D) return 0;
+ if (CXXConversionDecl *Conv = dyn_cast<CXXConversionDecl>(D)) {
+ return isLambdaConversionOperator(Conv);
+ }
+ else if (FunctionTemplateDecl *F = dyn_cast<FunctionTemplateDecl>(D)) {
----------------
No braces.
================
Comment at: include/clang/AST/ASTLambda.h:60-63
@@ +59,6 @@
+ else if (FunctionTemplateDecl *F = dyn_cast<FunctionTemplateDecl>(D)) {
+ if (CXXConversionDecl *Conv = dyn_cast_or_null<CXXConversionDecl>(
+ F->getTemplatedDecl())) {
+ return isLambdaConversionOperator(Conv);
+ }
+ }
----------------
Likewise.
================
Comment at: include/clang/AST/ASTLambda.h:59
@@ +58,3 @@
+ }
+ else if (FunctionTemplateDecl *F = dyn_cast<FunctionTemplateDecl>(D)) {
+ if (CXXConversionDecl *Conv = dyn_cast_or_null<CXXConversionDecl>(
----------------
No `else` after `return`
================
Comment at: include/clang/AST/ASTLambda.h:34-35
@@ +33,4 @@
+ DeclContext *LambdaClassDC = cast<DeclContext>(LambdaClass);
+ for (DeclContext::decl_iterator M = LambdaClassDC->decls_begin();
+ M != LambdaClassDC->decls_end(); ++M ) {
+ CXXMethodDecl *MD = 0;
----------------
You can simplify this (and make it more efficient) by using conversion_begin/conversion_end here.
================
Comment at: include/clang/AST/ASTLambda.h:85-87
@@ +84,5 @@
+ CXXRecordDecl *LambdaClass = MD->getParent();
+ if (LambdaClass && LambdaClass->isGenericLambda()) {
+ return isLambdaCallOperator(MD) && MD->isFunctionTemplateSpecialization();
+ }
+ return false;
----------------
No braces.
================
Comment at: include/clang/AST/ASTLambda.h:150
@@ +149,3 @@
+inline const DeclContext*
+getFirstNonLambdaEnclosingDeclContext(const DeclContext *DC) {
+ const CXXRecordDecl *Record = 0;
----------------
Maybe `getInnermostEnclosingNonLambdaContext`
================
Comment at: include/clang/AST/DeclCXX.h:538-540
@@ +537,5 @@
+ ///
+ /// We need a separate flag to identify a generic lambda because
+ /// this function can be called in a context where the CallOperator
+ /// has not been added to the lambda class, and lookup can fail.
+ bool IsGenericLambda : 1;
----------------
What function?
================
Comment at: include/clang/AST/DeclCXX.h:541
@@ +540,3 @@
+ /// has not been added to the lambda class, and lookup can fail.
+ bool IsGenericLambda : 1;
+
----------------
If you really need to store this, please remove a bit from somewhere else (maybe `NumCaptures`); adding this bit increases the size of `LmabdaDefinitionData` by 8 bytes on 64-bit systems.
================
Comment at: lib/Sema/SemaDecl.cpp:9353-9355
@@ +9352,5 @@
+ // has to be properly restored so that tryCaptureVariable doesn't try
+ // and capture any new variables. In addition when calculating potential
+ // captures during transformation of nested lambdas, it is necessary to
+ // have the LSI properly restored.
+ unsigned NumberLambdaScopesAdded = 0;
----------------
Why do you need to look at enclosing scopes here?
I would have expected that by the time we get to this point, we've already worked out the set of captures for the generic lambda, so if we odr-use any variable that's not already captured, we can just error out immediately (no need to look at any enclosing scopes). Could we instead push a scope that says "we're instantiating a generic lambda, it's an error if you discover that you need to capture anything new"?
================
Comment at: lib/Sema/SemaExpr.cpp:11047-11048
@@ -11045,2 +11046,4 @@
CXXConversionDecl *Conversion = cast<CXXConversionDecl>(MethodDecl);
+ CXXRecordDecl *LambdaClass = cast<CXXRecordDecl>(
+ Conversion->getParent());
if (Conversion->isLambdaToBlockPointerConversion())
----------------
Unused variable.
================
Comment at: lib/Sema/SemaExpr.cpp:11110-11119
@@ -11106,1 +11109,12 @@
}
+ } else if (getLangOpts().MicrosoftExt &&
+ Func->getTemplateSpecializationKind() == TSK_ExplicitSpecialization &&
+ isa<CXXMethodDecl>(Func) &&
+ cast<CXXMethodDecl>(Func)->getParent() == Func->getLexicalDeclContext()) {
+ // ok we must have an explicit specialization in microsoft mode
+ SourceLocation PointOfInstantiation = Loc;
+ PendingInstantiations.push_back(std::make_pair(Func,
+ PointOfInstantiation));
+ // Notify the consumer that a function was implicitly instantiated.
+ Consumer.HandleCXXImplicitFunctionInstantiation(Func);
+
----------------
This doesn't look like it should be part of this change?
================
Comment at: lib/Sema/SemaExpr.cpp:11256
@@ +11255,3 @@
+ SourceLocation Loc,
+ const bool BuildAndDiagnose, Sema &S) {
+
----------------
s/`BuildAndDiagnose`/`Diagnose`/ maybe?
================
Comment at: lib/Sema/SemaExpr.cpp:11886-11895
@@ +11885,12 @@
+ if (DefVD) {
+ if (DefVD->isWeak()) return false;
+ EvaluatedStmt *Eval = DefVD->ensureEvaluatedStmt();
+ if (Eval->CheckedICE)
+ // We have already checked whether this subexpression is an
+ // integral constant expression.
+ return Eval->IsICE;
+
+ const Expr *Init = cast<Expr>(Eval->Value);
+ if (Init->isValueDependent()) return true;
+ return DefVD->checkInitIsICE();
+ }
----------------
Can you replace this with
return Init->isValueDependent() || DefVD->checkInitIsICE();
================
Comment at: lib/Sema/SemaExpr.cpp:11989-12076
@@ -11750,1 +11988,90 @@
+ // generic lambdas and captures).
+ if (!IsPotentiallyEvaluatedContext(SemaRef)) {
+ if (!SemaRef.getLangOpts().CPlusPlus1y) return;
+ // Do not typically odr-use if we are not in at least a potentially
+ // evaluated context except if we are processing any generic lambdas
+ // or lambdas nested within generic lambdas.
+ if (SemaRef.isUnevaluatedContext()) return;
+
+ const bool refersToEnclosingScope =
+ (SemaRef.CurContext != Var->getDeclContext() &&
+ Var->getDeclContext()->isFunctionOrMethod());
+ if (!refersToEnclosingScope) return;
+
+ // template<class T> void f(T t) {
+ // const int x = 10;
+ // void foo(int, const int (&)[1]);
+ // void foo(const int&, const int (&)[2]);
+ // [=](auto a) {
+ // [=](auto b) {
+ // int selector[ sizeof(b) == 1 ? 1 : 2];
+ // foo(x, selector);
+ // };
+ // };
+ if (LambdaScopeInfo *const LSI = SemaRef.getCurLambda()) {
+ // If a variable can potentially be non-odrused, defer determination of
+ // its actual odr-use status until we instantiate.
+ // - but add to the list of potential captures, so enclosing
+ // lambdas can capture it.
+ //
+ const bool IsPotentiallyNonODRUsedConstant =
+ IsVariablePotentiallyAConstantExpression(Var, E, SemaRef);
+ if (LSI->ImpCaptureStyle != LambdaScopeInfo::ImpCap_None) {
+ // Add the potential capture - which will be checked by
+ // all enclosing lambdas unless it involves a reference
+ // initialized by a constant expression (which can
+ // never be odr-used).
+
+ if (!(IsPotentiallyNonODRUsedConstant &&
+ Var->getType()->isReferenceType()))
+ LSI->addPotentialCapture(Var, E->IgnoreParens());
+ }
+ // If this lambda can never capture, don't diagnose errors
+ // for variables that could potentially not be odr-used for
+ // certain instantiations - until the lambda's call operator
+ // is actually instantiated.
+ // (LSI->ImpCaptureStyle == LambdaScopeInfo::ImpCap_None)
+ else if (IsPotentiallyNonODRUsedConstant)
+ return;
+
+ // Even if the enclosing context is dependent, we can still
+ // diagnose the use of variables that must be odr-used/captured
+ // by the lambda. But do not mark them odr-used just yet.
+ DeclContext *EnclosingFunctionCtx = SemaRef.CurContext->
+ getParent()->getParent();
+ if (EnclosingFunctionCtx->isDependentContext()) {
+ if (IsPotentiallyNonODRUsedConstant) return;
+ QualType CaptureType, DeclRefType;
+ if (SemaRef.tryCaptureVariable(Var, Loc, Sema::TryCapture_Implicit,
+ /*EllipsisLoc*/ SourceLocation(), /*BuildAndDiagnose*/ false,
+ CaptureType, DeclRefType, 0))
+ SemaRef.tryCaptureVariable(Var, Loc, Sema::TryCapture_Implicit,
+ /*EllipsisLoc*/ SourceLocation(), /*BuildAndDiagnose*/ true,
+ CaptureType, DeclRefType, 0);
+ return;
+ } else {
+ // Make Sure that we can capture this PotentiallyNonODRUsedConstant,
+ // in all enclosing lambdas, before we try and capture it in this one.
+ if (IsPotentiallyNonODRUsedConstant) {
+ unsigned CapturableFunctionScopeIndex = 0;
+ if (!GetNearestEnclosingCapturableLambda(LSI->CallOperator,
+ SemaRef.FunctionScopes, CapturableFunctionScopeIndex,
+ SemaRef.CurContext, Var))
+ return;
+ // If this lambda is not the one that can capture this Var, skip
+ // marking the variable used (i.e. capturing the variable in the
+ // lambda)
+ if (CapturableFunctionScopeIndex !=
+ (SemaRef.FunctionScopes.size() - 1))
+ return;
+ }
+ }
+ // Since the enclosing function ctx is non-dependent, process the
+ // lambda's captures and mark them odr-used or diagnose an error
+ // if necessary.
+ } else
+ // Not in a lambda, but in a dependent context; so don't mark odr-use.
+ return;
+ }
+
----------------
This seems rather inelegant. Alternative approach:
* In the "not sure" case, just treat the lambda as if it captures the entity.
* When the surrounding generic lambda is instantiated, recompute the captures of the inner lambda (more generally, act as if we were building a brand new lambda in the instantiation of the surrounding generic lambda).
http://llvm-reviews.chandlerc.com/D1174
More information about the cfe-commits
mailing list