[PATCH] Simple Generic Lambdas: Chicago Reversion
Faisal Vali
faisalv at yahoo.com
Sat Nov 9 20:40:11 PST 2013
Most (if not all) of the functionality included in this code (including all the various revisions) has been committed via separated-out generic lambda related patches.
Closing this revision.
================
Comment at: lib/Sema/SemaExpr.cpp:11993
@@ +11992,3 @@
+ if (!IsPotentiallyEvaluatedContext(SemaRef)) {
+ if (!SemaRef.getLangOpts().CPlusPlus1y) return;
+ // Do not typically odr-use if we are not in at least a potentially
----------------
remove this check
================
Comment at: include/clang/Sema/ScopeInfo.h:695
@@ +694,3 @@
+ }
+ void removePotentialCapture(Expr *E) {
+ for (PotentialODRUseVectorTy::iterator I = PotentialCaptures.begin();
----------------
This is O(n2) - consider optimizing
================
Comment at: include/clang/Sema/SemaLambda.h:41
@@ +40,3 @@
+inline bool
+isVariableAlreadyCapturedByLambdaClass(VarDecl *Var,
+ CXXRecordDecl *LambdaClass) {
----------------
This is inefficient - because we are recreating the densemap for each lookup - avoid it - so prefer to use primed lambdaascopeinfo
================
Comment at: include/clang/Sema/SemaLambda.h:52
@@ +51,3 @@
+ QualType TypeToReplaceAutoWith, Sema &S) {
+ TypeToReplaceAutoWith = TypeToReplaceAutoWith->getCanonicalTypeInternal();
+ QualType AutoResultType = F->getResultType(); //->getCanonicalTypeInternal();
----------------
This is disturbing to Doug why canonical type is necessary - if we figure out why it is so, prefer to use ASTContext.getCanonicalType(TypeToReplaceAutoWIth)
================
Comment at: lib/Sema/SemaExprCXX.cpp:5736
@@ +5735,3 @@
+ CurrentLSI->ImpCaptureStyle != CapturingScopeInfo::ImpCap_None &&
+ isLambdaWithinGenericLambdaDeclContext(CurContext)) {
+
----------------
What conditions are some of these checks necessary - and why can't they be assertions:
- isLambdaWithin, unevaluated, isdepnedent, implicitcapture
================
Comment at: lib/AST/Decl.cpp:1016
@@ -1009,1 +1015,3 @@
+ getInnermostEnclosingNonLambdaContext(DC);
+ if (const NamedDecl *ND = dyn_cast<NamedDecl>(NonLambdaEnclosingDC)) {
return getLVForDecl(ND, computation);
----------------
Per Doug - why does this not already work - is this a bug with nested nongeneric lambdas etc.. please provide an example ...
================
Comment at: lib/Sema/SemaDecl.cpp:9305
@@ -9284,1 +9304,3 @@
+ if (LE->getCaptureDefault() == LCD_None)
+ LSI->ImpCaptureStyle = CapturingScopeInfo::ImpCap_None;
----------------
Use Switch to ensure all cases are covered
================
Comment at: lib/Sema/SemaExpr.cpp:12061
@@ +12060,3 @@
+ unsigned CapturableFunctionScopeIndex = 0;
+ if (!GetInnermostEnclosingCapturableLambda(LSI->CallOperator,
+ SemaRef.FunctionScopes, CapturableFunctionScopeIndex,
----------------
Per Doug - this might be replaceable by a tryCaptureVar in non-build/diagnostic mode seeded with the functionscopindex one above(enclosing) lambda scope info - and if so can we merge these two if/else branches
================
Comment at: lib/Sema/SemaExpr.cpp:11933
@@ +11932,3 @@
+ if (LambdaScopeInfo *LSI = getCurLambda()) {
+ for (int I = LSI->getNumPotentialCaptures(); I--; ) {
+ Expr *ExprVarUsedIn = 0;
----------------
The complexity of this loop should be improved by the use of remove_if and erase and make this linear complexity big o
================
Comment at: lib/Sema/SemaExprMember.cpp:864
@@ +863,3 @@
+ if (!BaseExpr && CurLSI) {
+ DeclContext *NonLambdaParentDC = getFunctionLevelDeclContext();
+ // If the parent non-lambda function is non-dependent ...
----------------
Would be better to also diagnose 'this' capture violations in member templates if we can unambioguously do so, therefore the check for enclosing function being dependent might not be the better choice
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:3763
@@ +3762,3 @@
+
+ const FunctionType* ToFunType = ToTypePtr->isFunctionType() ?
+ ToTypePtr->getAs<FunctionType>() : 0;
----------------
Just use getas<FunctionType> -
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:3795
@@ +3794,3 @@
+ // matches the return type of the call operator.
+ if (TemplateDeductionResult Result
+ = DeduceTemplateArgumentsByTypeMatch(*this, TemplateParams,
----------------
Consider simplifying this into a simple type comparison through the ASTContext.isSameType()
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:3816
@@ +3815,3 @@
+ FunctionProtoType::ExtProtoInfo EPI = InvokerFPT->getExtProtoInfo();
+ EPI.TypeQuals = 0;
+ InvokerSpecialization->setType(Context.getFunctionType(
----------------
The Invoker function should not have a const qualifier - this tended to work in the non-generic lambda setting because type comparisons were not as necessary - but it is important in the generic lambda setting - consider in semalambda.cpp using getTrivialTypeSourceInfo using the lambda Call operator's type modulo constness when creating the static invoker method
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:41
@@ +40,3 @@
+ // - leave it as is?
+ static clang::CXXConversionDecl *CurGenericLambdaConversionOpUndergoingDeduction;
+ }
----------------
The need for this variable can be avoided byleveraging the code for overload resolution/deduction from address of function template with auto return type being assigned to a pointer to function
template<class T> auto foo(T t) { return t; }
char (*fp)(char) = foo; -- channel the code through this logic.
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:923
@@ +922,3 @@
+ // set here for non-generic lambdas. Is this safe to do?
+ if (!CallOperator->getDescribedFunctionTemplate())
+ CallOperator->setInstantiationOfMemberFunction(E->getCallOperator(),
----------------
Must add else {
CallOperator->setInstantiationOfMemberTemplate(TieToPrimaryTemplate, TSK_ImplicitInstantiation)
}
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:928
@@ -919,2 +927,3 @@
}
-
+ TemplateParameterList *TransformTemplateParameterList(
+ TemplateParameterList *OrigTPL) {
----------------
Pass in the DeclContext of the ParameterList (which for lambdas should be the call operator) and avoid the special case of a template parameter list of 0
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:4115
@@ +4114,3 @@
+ // FVQUESTION? Can I make this check more general?
+ if (isParmVarDecl && (isLambdaWithinGenericLambdaDeclContext(CurContext) ||
+ isGenericLambda(CurContext)) && !ParentDC->isDependentContext()) {
----------------
Hoist the !dependentparentDC check that entails return of the Decl itself higher versus these special cases
================
Comment at: lib/Sema/TreeTransform.h:599
@@ +598,3 @@
+ TemplateParameterList *TPL) {
+ // FVQUESTION: Currently not implemented - Check with Doug/Richard
+ // what sort of semantics we want the default one to have?
----------------
add llvm_unreachable("....")
================
Comment at: lib/Sema/TreeTransform.h:2044
@@ -2031,1 +2043,3 @@
bool isImplicit) {
+ DiagnosticsEngine &DE = getSema().getDiagnostics();
+ DiagnosticErrorTrap DetectNewError(DE);
----------------
Instead of using the DiagnosticErrorTracker - consider modifying checkCXXThisCapture to return a flag that indicates success or failure
================
Comment at: lib/Sema/TreeTransform.h:4582
@@ +4581,3 @@
+ // into a non-dependent type.
+ if (!T->isInstantiationDependentType() && !T->isVariablyModifiedType()) {
+ DecltypeTypeLoc NewTL = TLB.push<DecltypeTypeLoc>(TL.getType());
----------------
Remove the test for isVariablyModifiedType
Per Doug - this is worrisome that we need to handle this case here - we should be partial instatiation aware for decltype - explore a better solution here - this has to do with decltypes referring to outer lambda parameters
================
Comment at: lib/Serialization/ASTWriter.cpp:5137
@@ -5134,2 +5136,3 @@
AddTypeSourceInfo(Lambda.MethodTyInfo, Record);
+ AddStmt(Lambda.TheLambdaExpr);
for (unsigned I = 0, N = Lambda.NumCaptures; I != N; ++I) {
----------------
Remove LambdaExpr from CXXRecordDecl to avoid incorrect/circularity in serialization
http://llvm-reviews.chandlerc.com/D1174
More information about the cfe-commits
mailing list