Page MenuHomePhabricator

Always build a new TypeSourceInfo for function templates with parameters
ClosedPublic

Authored by thakis on Jan 22 2016, 11:00 AM.

Details

Reviewers
rnk
rsmith
Summary

RecursiveASTVisitor::TraverseFunctionHelper() traverses a function's ParmVarDecls by going to the function's getTypeSourceInfo if it exists, and `
DEF_TRAVERSE_TYPELOC(FunctionProtoType` then goes to the function's ParmVarDecls.

For a function template that doesn't have parameters that explicitly depend on the template parameter, we used to be clever and not build a new TypeSourceInfo. That meant that when an instantiation of such a template is visited, its TypeSourceInfo would point to the ParmVarDecls of the template, not of the instantiation, which then confused clients of RecursiveASTVisitor.

So don't be clever for function templates that have parameters, even if none of the parameters depend on the type.

Fixes PR26257.

Diff Detail

Event Timeline

thakis retitled this revision from to Always build a new TypeSourceInfo for function templates with parameters.
thakis updated this object.
thakis added reviewers: rsmith, rnk.
thakis added a subscriber: cfe-commits.

rnk, I added you because you hacked in this general area a while ago in r187528. Since Richard is out, do you think you can take a look?

rnk edited edge metadata.Jan 28 2016, 12:39 PM

The fact that an instantiated type might point to decls from the template pattern seems like expected behavior. The parts of the template that are the same are supposed to be shared.

Can we dig in to what is going wrong in parent map construction?

In D16478#338653, @rnk wrote:

The fact that an instantiated type might point to decls from the template pattern seems like expected behavior. The parts of the template that are the same are supposed to be shared.

Can we dig in to what is going wrong in parent map construction?

"RecursiveASTVisitor::TraverseFunctionHelper() traverses a function's ParmVarDecls by going to the function's getTypeSourceInfo if it exists, and `
DEF_TRAVERSE_TYPELOC(FunctionProtoType` then goes to the function's ParmVarDecls." was my attempt to do that. I guess I can expand this a bit with code snippets.

The parent map uses a RecursiveASTVisitor to build parents: It keeps a stack of nodes it has seen so far, and when something gets visited, it marks parent[something] = seenstack.top(). RecursiveASTVisitor does this for functions:

DEF_TRAVERSE_DECL(FunctionDecl, {
  // We skip decls_begin/decls_end, which are already covered by
  // TraverseFunctionHelper().
  return TraverseFunctionHelper(D);
})

TraverseFunctionHelper goes to the TypeSource if it exists to visit the parameter decls:

bool RecursiveASTVisitor<Derived>::TraverseFunctionHelper(FunctionDecl *D) {
  // ...
  if (TypeSourceInfo *TSI = D->getTypeSourceInfo()) {
    TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));

The TypeLoc then goes to the param's decl if available:

DEF_TRAVERSE_TYPELOC(FunctionProtoType, {
  TRY_TO(TraverseTypeLoc(TL.getReturnLoc()));

  const FunctionProtoType *T = TL.getTypePtr();

  for (unsigned I = 0, E = TL.getNumParams(); I != E; ++I) {
    if (TL.getParam(I)) {

So when the instantiated CXXMethodDecl for quantizedSize is visited, this goes and visits the ParmDecls from the template CXXMethodDecl, not from the instantiated one. Hence, the instantiated ParmDecl for count never gets a parent assigned (since it's never visited).

So at least RecursiveASTVisitor didn't expect the current behavior.

The instantiated does get a new collection of ParmVarDecls while getting
the pattern Type which points to the pattern ParmVarDecls. So the
ParmVarDecls pointed in the instantiated are not the same as those pointed
by its Type.

Traversing by Type or by the Decl finds a different set of ParmVarDecls
which seems an inconsistency or at least a surprise.

rsmith edited edge metadata.Feb 1 2016, 1:05 PM

This is a bug -- we intend to reuse Stmt nodes across the template and its instantiations, but we should have separate Decl nodes in each instantiation (otherwise the Decl's parent will be wrong etc).

lib/Sema/SemaTemplateInstantiate.cpp
1519–1521

This doesn't look quite right: IIRC, there can be attributes here as well as parens (in particular, calling convention attributes can appear in this position).

1526

Just return true here?

thakis updated this revision to Diff 46577.Feb 1 2016, 1:57 PM
thakis edited edge metadata.

just return true

thakis marked an inline comment as done.Feb 1 2016, 1:58 PM
thakis added inline comments.
lib/Sema/SemaTemplateInstantiate.cpp
1519–1521

Ack.

thakis updated this revision to Diff 46578.Feb 1 2016, 2:01 PM

restore accidentally dropped test

rsmith accepted this revision.Feb 1 2016, 2:14 PM
rsmith edited edge metadata.

LGTM

lib/Sema/SemaTemplateInstantiate.cpp
1525–1530

Maybe replace the whole loop with

for (ParmVarDecl *D : FP.getParmArray())
  if (D)
    return true;

(or an STL algorithm for same)?

This revision is now accepted and ready to land.Feb 1 2016, 2:14 PM
thakis closed this revision.Feb 1 2016, 2:37 PM

r259428, thanks! Went with the for-each loop but left the structure as-is -- I find the comments helpful and it's not clear where they should go else.