[PATCH] Implement transformations of non-capturing nested lambdas.
Faisal Vali
faisalv at yahoo.com
Mon Oct 7 20:53:10 PDT 2013
Thanks for the feedback Richard - Look forward to your response!
================
Comment at: lib/Sema/TreeTransform.h:8329-8343
@@ -8293,1 +8328,17 @@
+ // Use the Old Call Operator's TypeSourceInfo if it is already transformed.
+ if (CallOpWasAlreadyTransformed)
+ NewCallOpTSI = OldCallOpTSI;
+ else {
+ // Transform the TypeSourceInfo of the Original Lambda's Call Operator.
+ // The transformation MUST be done in the CurrentInstantiationScope since
+ // it introduces a mapping of the original to the newly created
+ // transformed parameters.
+
+ TypeLocBuilder NewCallOpTLBuilder;
+ QualType NewCallOpType = TransformFunctionProtoType(NewCallOpTLBuilder,
+ OldCallOpFPTL,
+ 0, 0);
+ NewCallOpTSI = NewCallOpTLBuilder.getTypeSourceInfo(getSema().Context,
+ NewCallOpType);
}
+ // Extract the ParmVarDecls from the NewCallOpTSI and add them to
----------------
Richard Smith wrote:
> This looks equivalent to just:
>
> NewCallOpTSI = getDerived().TransformType(OldCallOpTSI);
It's not because using TransformType maps the original parameters to the instantiated parameters in an instantiation scope that gets popped off. This was the source of me introducing that decltype kludge since digging into the decltype(a) resulted in 'a' not being found (it would get popped of once the typesourceinfo was transformed) which is now not necessary because of the current version which calls TransformFunctionProtoType to create mappings in the CurrentInstantiationScope which does not get popped off and is found by nested lambdas undergoing transformation.
================
Comment at: lib/Sema/TreeTransform.h:8348-8362
@@ +8347,17 @@
+ // lambda call operator to the CurrentInstantiationScope.
+ SmallVector<ParmVarDecl *, 4> Params;
+ {
+ FunctionProtoTypeLoc NewCallOpFPTL =
+ NewCallOpTSI->getTypeLoc().getAs<FunctionProtoTypeLoc>();
+ ParmVarDecl **NewParamDeclArray = NewCallOpFPTL.getParmArray();
+ const unsigned NewNumArgs = NewCallOpFPTL.getNumArgs();
+
+ for (unsigned I = 0; I < NewNumArgs; ++I) {
+ if (CallOpWasAlreadyTransformed)
+ SemaRef.CurrentInstantiationScope->InstantiatedLocal(
+ NewParamDeclArray[I],
+ NewParamDeclArray[I]);
+ Params.push_back(NewParamDeclArray[I]);
+ }
+ }
+
----------------
Richard Smith wrote:
> ... and with this piece, this looks like
>
> NewCallOpTSI = SubstFunctionType(E->getCallOperator(), Params);
>
> (`SubstFunctionType` is on `TemplateDeclInstantiator`.) You're also missing the pack instantiation case here. For instance, given:
>
> template<int...N> void f() {
> [](auto ...a[N]) {};
> }
>
> we need to instantiate `a` into a pack here.
SubstFunctionType does not work because once again it pops off the instantiation scope it introduces the parameter mappings (which are needed to be in scope when transforming the body of the lambda). I guess I could use SubstFunctionType, and then add the instantiated arguments using addInstantiatedParametersToScope? But this does seem to work, and seems less convoluted ;)
In regards to the variadic example - are you sure that should compile, shouldn't 'N' need to be expanded?
Also the variadic pack case should always return false to AlreadyTransformed (since it will always return true to isInstantiationDependent) - the only time it won't is if it has already been specialized and has no dependence on any template parameters, right?
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:950-953
@@ +949,6 @@
+ OldCallOperatorTemplate);
+ // Set this as a specialization so we don't go digging into the
+ // OldCallOperatorTemplate when retrieving the
+ // 'FunctionDecl::getTemplateInstantiationPattern()'
+ NewCallOperatorTemplate->setMemberSpecialization();
+ }
----------------
Richard Smith wrote:
> It's a bit nasty to (essentially) set this bit incorrectly. Can you put the check into getTemplateInstantiationPattern instead?
but if we are relating NewCallOpTemplate with OldCallOpTemplate and claiming one is instantiated from the other - if you look at the documentation for isMemberSpecialization() - this does not seem entirely an abuse of setting this bit? The outer template (be it a generic lambda, a function template or a class template) has been specialized to some degree, and so the inner call operator template is a specialization in the sense X<int>::Inner is in the documentation for setMemberSpecialization - at least that's how i tried to rationalize it. But I seem to be missing something...
================
Comment at: lib/Sema/TreeTransform.h:8328
@@ +8327,3 @@
+
+ // Use the Old Call Operator's TypeSourceInfo if it is already transformed.
+ if (CallOpWasAlreadyTransformed)
----------------
Richard Smith wrote:
> Strange capitalization here?
Sorry - I'm not sure i'm picking up on it - what would you suggest?
================
Comment at: lib/Sema/TreeTransform.h:8357
@@ +8356,3 @@
+ if (CallOpWasAlreadyTransformed)
+ SemaRef.CurrentInstantiationScope->InstantiatedLocal(
+ NewParamDeclArray[I],
----------------
Richard Smith wrote:
> This is very instantiation-specific. It's not really OK to do this in TreeTransform. You should instead move this out into a hook that we can handle in a template-instantiation-specific way.
aah - transformedLocalDecl should fit the bill right?
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:960-961
@@ +959,4 @@
+ TemplateParameterList *NewTPL = 0;
+ if (OrigTPL) {
+ if (!OrigTPL->size()) return OrigTPL; // size 0, do nothing
+
----------------
Richard Smith wrote:
> Fold these together into
>
> if (!OrigTPL || !OrigTPL->size()) return OrigTPL;
done - thanks!
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:4192-4193
@@ +4191,4 @@
+ // fooT('a');
+ // ... without this check below, findInstantiationOf fails with
+ // an assertion violation.
+ if (isa<ParmVarDecl>(D) && !ParentDC->isDependentContext() &&
----------------
Richard Smith wrote:
> Drop this line. It's enough for the check to be correct; you don't need to explain what would happen if we didn't have it.
ok - your fix below makes sense. Eitherway this is not necessary as long as the paramaters of the lambda are mapped in the right instantiation scope (pls see below why TransformType did not work or SubstFunctionType might require additional effort to make it work)
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:4195
@@ +4194,3 @@
+ if (isa<ParmVarDecl>(D) && !ParentDC->isDependentContext() &&
+ !cast<ParmVarDecl>(D)->getType()->isInstantiationDependentType())
+ return D;
----------------
Richard Smith wrote:
> An instantiation dependent type should not be possible here if the parent is not a dependent context.
OK.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:4198
@@ -4174,2 +4197,3 @@
+
if (isa<ParmVarDecl>(D) || isa<NonTypeTemplateParmDecl>(D) ||
isa<TemplateTypeParmDecl>(D) || isa<TemplateTemplateParmDecl>(D) ||
----------------
Richard Smith wrote:
> I think a simpler way of making the above change is to just remove the ParmVarDecl case here. Dependent parameters are caught by the
>
> (ParentDC->isFunctionOrMethod() && ParentDC->isDependentContext())
>
> check here, and non-dependent parameters will fall through to the 'return D;' case below.
OK.
================
Comment at: lib/Sema/TreeTransform.h:4590
@@ +4589,3 @@
+ //};
+ if (!T->isInstantiationDependentType()) {
+ DecltypeTypeLoc NewTL = TLB.push<DecltypeTypeLoc>(TL.getType());
----------------
Richard Smith wrote:
> It's not OK for TreeTransform to look at the dependence of things. It's not just used for instantiation.
OK - this kludge can be removed, pls see below for my thoughts on why this now works ...
================
Comment at: lib/Sema/TreeTransform.h:4582-4594
@@ -4575,2 +4581,15 @@
const DecltypeType *T = TL.getTypePtr();
+ // Don't transform a decltype construct that has already been transformed
+ // into a non-dependent type.
+ // Allows the following to compile:
+ // auto L = [](auto a) {
+ // return [](auto b) ->decltype(a) {
+ // return b;
+ // };
+ //};
+ if (!T->isInstantiationDependentType()) {
+ DecltypeTypeLoc NewTL = TLB.push<DecltypeTypeLoc>(TL.getType());
+ NewTL.setNameLoc(TL.getNameLoc());
+ return NewTL.getType();
+ }
----------------
Richard Smith wrote:
> Is this change necessary, given your fix to `FindInstantiatedDecl`? There doesn't seem to be anything specific to `decltype` in this example. If this doesn't work, I'd expect
>
> auto L = [](auto a) {
> return [](auto b) -> int (&)[sizeof(a)] { ... }
> }
>
> to fail in the same way.
OK - pls see below
================
Comment at: lib/Sema/TreeTransform.h:8322
@@ +8321,3 @@
+ FunctionProtoTypeLoc OldCallOpFPTL =
+ OldCallOpTSI->getTypeLoc().getAs<FunctionProtoTypeLoc>();
+ TypeSourceInfo *NewCallOpTSI = 0;
----------------
Richard Smith wrote:
> castAs.
aah - getAs is deprecated?
http://llvm-reviews.chandlerc.com/D1784
More information about the cfe-commits
mailing list