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
