On Jul 30, 2012, at 5:35 AM, Erik Olofsson <[email protected]> wrote:
> I have taken a stab at this with the attached patch.
>
> At the exit of each LocalInstantiationScope we save all instantiations to a
> permanent instantiation scope for their parent function.
>
> When an instantiation of a function is on the stack we create the permanent
> instantiation scope for this function and link this permanent scope to it's
> local LocalInstantiationScope on the stack. We also look for a parent
> function of this function and link it into the permanent scope stack. We also
> take care to link the current stack scope for the function because we may
> need to restore this scope recursively and then we need the local stack scope
> in the chain as well. This is handled with FunctionInInstantiationScope
> helper.
>
> We then use RestoreLocalInstantiationScope together with setupLocalScope to
> restore the function local scope chain for the instantiations that require it.
>
> I also fixed bugs in the parser that got template depth wrong with recursive
> function-local scopes.
>
> Test cases were added that includes the cases of local scopes not working I
> found during the development of this.
>
> Blocks don't seem to be functions internally so they will probably not work
> correctly with function local contexts.
Right, blocks are BlockDecls. We don't have to deal with blocks in the first
implement of this.
> While I was developing this I sometimes ended up with a static_assert that
> was still dependant and the compiler didn't complain about this just silently
> ignored it, I was not sure how to fix this but just thought I would mention
> it.
Interesting. Do you have an example of this that we could poke at?
Very nice patch. A couple comments, most trivial and some more substantial,
@@ -352,6 +369,47 @@
unsigned *NumExplicitArgs = 0)
const;
};
+ /// \brief Keeps track of overrides for local instantiation scopes
+ /// used to reset the scope when instantiating dependant types defined
+ /// in function local contexts.
Typo "dependant"; should be "dependent".
+ /// \brief Keeps track of linking permanent function scopes into the
currently
+ /// instanciating scope. This is needed because the permanent scope might be
+ /// recursively overridden.
Typo "instanciating".
+void FunctionInInstantiationScope::functionInScope(
+ LocalInstantiationScope *Scope, FunctionDecl *D)
{
+ assert(LinkedSavedFunctionScope == 0 && "Function already in scope");
+ Sema::SavedLocalScope &SavedScope = SemaRef.SavedLocalFunctionScopes[D];
+ if (!SavedScope.SaveScope)
+ SavedScope.SaveScope = new LocalInstantiationScope(SemaRef,
+ /*CombineWithOuterScope=*/true, /*IsSaved=*/true);
+ assert((!D->getTemplateInstantiationPattern()
+ || D->getTemplateInstantiationPattern() != D)
+ && "Can only be used on instantiations");
+
+ const FunctionDecl *ParentFunc = cast_or_null<const FunctionDecl>(
+ D->getParentFunctionOrMethod());
Shouldn't need the "const" in "const FunctionDecl".
Decl *Sema::SubstDecl(Decl *D, DeclContext *Owner,
const MultiLevelTemplateArgumentList &TemplateArgs) {
- TemplateDeclInstantiator Instantiator(*this, Owner, TemplateArgs);
- if (D->isInvalidDecl())
- return 0;
+
+ RestoreLocalInstantiationScope RestoreLocalScope(*this);
+ if (setupLocalScope(RestoreLocalScope, dyn_cast_or_null<Decl>(D),
+ dyn_cast_or_null<Decl>(Owner))) {
+ LocalInstantiationScope Scope(*this, /*CombineWithOuterScope=*/true);
- return Instantiator.Visit(D);
+ TemplateDeclInstantiator Instantiator(*this, Owner, TemplateArgs);
+ if (D->isInvalidDecl())
+ return 0;
+
+ return Instantiator.Visit(D);
+ }
+ else {
+ TemplateDeclInstantiator Instantiator(*this, Owner, TemplateArgs);
+ if (D->isInvalidDecl())
+ return 0;
+
+ return Instantiator.Visit(D);
+ }
}
Since the "if" always returns, please de-nest the "else".
+void LocalInstantiationScope::saveDeclInPermanentScope(Decl const *D,
+ Decl *Inst) {
+ FunctionDecl *ParentFunction = dyn_cast_or_null<FunctionDecl>(
+ Inst->getParentFunctionOrMethod());
+ if (ParentFunction) {
+
+ FunctionDecl *Pattern =
+ ParentFunction->getTemplateInstantiationPattern();
+ if (Pattern && Pattern != ParentFunction)
+ {
We tend to prefer hanging braces, e.g.,
if (Pattern && Pattern != ParentFunction) {
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp (revision 160944)
+++ lib/Parse/ParseStmt.cpp (working copy)
@@ -2043,7 +2043,14 @@
// Do not enter a scope for the brace, as the arguments are in the same scope
// (the function body) as the body itself. Instead, just read the statement
// list and put it into a CompoundStmt for safe keeping.
+
+ // Increase depth for any templates specified inside of local classes
+ unsigned OldDepth = TemplateParameterDepth;
+ if (Decl && Decl->isTemplateDecl())
+ ++TemplateParameterDepth;
+
StmtResult FnBody(ParseCompoundStatementBody());
+ TemplateParameterDepth = OldDepth;
// If the function body could not be parsed, make a bogus compoundstmt.
if (FnBody.isInvalid()) {
I don't quite understand this. Local classes cannot have member templates, so
what exactly are we adjusting for here?
+
+ llvm::DenseMap<const FunctionDecl *, SavedLocalScope>
+ SavedLocalFunctionScopes;
+
This map isn't being persisted in the serialized AST format. In practice, that
isn't going to be a problem, because precompiled headers don't instantiate
function definitions [*], and people don't tend to use AST files for "complete"
translation units in ways that trigger more instantiations. Still, a note here
describing why we don't have to serialize this information would be helpful for
our future selves.
My only other general comment is that this is a lot of information to save for
template instantiations, but it's only used in the (relatively uncommon) case
where there is a local class within a function template. Can we only save this
information when the pattern from which the function is instantiated actually
has a local class somewhere in it?
Thanks for working on this!
- Doug
[*] Except for constexpr function definitions, but those don't have local
classes.
> // Erik
> <PR9685.patch>
>
> On 2011-10-19, at 07:20, Douglas Gregor <[email protected]> wrote:
>
>>
>> On Oct 17, 2011, at 10:45 PM, David Blaikie wrote:
>>
>>> I'll be the first to admit I'm not entirely confident that this fix is the
>>> right one - but at the very least it's a starting point. It fixes the bug
>>> and doesn't regress any other clang tests.
>>>
>>> If there are things this change breaks, I'll be happy to add tests for
>>> those cases & work on a fix that accommodates them, or if there's just a
>>> more appropriate way to express the required semantics, I'm all ears.
>>>
>>> [basically, members of local structs were being instantiated in the context
>>> of their reference, not their definition, and failing because the Decl
>>> corresponding to the template of the member wasn't in the context of the
>>> instantiation scope (forEach in the attached example). By extending the
>>> lifetime of the LocalInstantiationScope and walking up past uncombined
>>> scopes in LocalInstantiationScope::findInstantiationOf it's now able to
>>> find the relevant Decl & instantiate correctly]
>>
>> I see why this appears to be working, but it's not actually the right fix.
>> Let's walk into what it's actually doing:
>>
>> Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
>> ===================================================================
>> --- lib/Sema/SemaTemplateInstantiateDecl.cpp (revision 142336)
>> +++ lib/Sema/SemaTemplateInstantiateDecl.cpp (working copy)
>> @@ -2540,7 +2540,6 @@
>> // This class may have local implicit instantiations that need to be
>> // instantiation within this scope.
>> PerformPendingInstantiations(/*LocalOnly=*/true);
>> - Scope.Exit();
>>
>> This tells Clang not to exit the instantiation scope, i.e., the thing that
>> maps from entities in the template to entities that map into the current
>> instantiation. So, we'll end up leaving the mapping for this instantiation
>> (doIt<int>) available while we instantiate other templates (including
>> completely unrelated templates whose instantiation happens to have shown up
>> in the list of instantiations).
>>
>> Now let's look at:
>>
>> Index: lib/Sema/SemaTemplateInstantiate.cpp
>> ===================================================================
>> --- lib/Sema/SemaTemplateInstantiate.cpp (revision 142336)
>> +++ lib/Sema/SemaTemplateInstantiate.cpp (working copy)
>> @@ -2291,10 +2291,6 @@
>> else
>> CheckD = 0;
>> } while (CheckD);
>> -
>> - // If we aren't combined with our outer scope, we're done.
>> - if (!Current->CombineWithOuterScope)
>> - break;
>> }
>>
>> This change tells Clang to keep looking into outer instantiation scopes when
>> it can't find what it's looking for in the current instantiation scope. That
>> CombineWithOuterScope barrier is important, however, to make sure that we
>> don't ever end up using bindings from other instantiations of the same
>> template that are further up on the stack. This can become problematic with,
>> e.g., label declarations, which are instantiated on demand.
>>
>> I don't have a killer test case for you that breaks when you make this
>> change; I suspect that that test case is out there, but that it is going to
>> be very hard to write. However, let's see how things went wrong:
>>
>> - In Sema::MarkDeclarationReferenced, not how around line 9365 we check
>> whether the context of an instantiable function is a local class and, if so,
>> push it into the set of pending *local* instantiations rather than the set
>> of pending (global) instantiations).
>> - In Sema::InstantiateFunctionDefinition, we create a
>> LocalInstantiationScope and again check for a local class. If we're
>> instantiating something in a local class, then we merge our current
>> instantiation scope with its parent scope, which turns on that
>> CombineWithOuterScope flag (i.e., skipping the break in the second patch
>> snippet above).
>> - Late in Sema::InstantiateFunctionDefinition, we instantiate all of the
>> pending local instantiations while we're still inside the instantiation
>> scope of the original function. So, in combination with the previous bullet,
>> this means that those instantiations can access our mappings from local
>> names (e..g, Functor) to their instantiations (the instantiated Functor).
>>
>> Now, things break in your test case because we're forEach<Functor> is not
>> classified as a local instantiation, so it doesn't get instantiated while
>> we're still inside doIt<int>'s scope. Hence, when forEach<Functor> then
>> triggers an instantiation of doIt<int>::Functor::operator(), we're not longer
>>
>> What your patch is doing is essentially treating *all* instantiations like
>> they are local instantiations, so everything gets instantiated while
>> doIt<int> is on the current instantiation stack, and all of the
>> instantiation scopes are effectively merged.
>>
>> I *think* the right answer is that, when there are local classes involved in
>> the instantiation, we're going to want to keep around the contents of the
>> LocalInstantiationScope in some side data structure. Then, when we're
>> instantiating something that somehow refers to the local class (or a member
>> of it), we'll have to re-create a LocalInstantiationScope as a parent
>> LocalInstantiationScope, so that the right set of bindings are available.
>>
>> - Doug
>>
>> _______________________________________________
>> cfe-commits mailing list
>> [email protected]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
