[AST] RAV doesn't traverse explicitly instantiated function bodies by default

[sammccall]

Summary

This is consistent with the idea of shouldVisitTemplateInstantiations():
when false, we should traverse the code the user wrote but not the code
clang synthesized. Therefore we should be able to traverse the
signature of the function without traversing its body.
This is also consistent with how class and variable templates behave.

Note that currently RAV does not traverse *any* part of the these when
shouldVisitTemplateInstantiations() is false (which is another bug).
So currently this change is only observable when the user passes the
instantiated FunctionDecl to TraverseDecl themselves.

This is what the caller in CodeGenPGO does, and it wants to traverse the
instantiated body. I believe not traversing instantiations in general in
this context is an oversight.

[sammccall]

@davidxl does the one-line CodeGenPGO change look reasonable? If you're not the person to ask, could you recommend someone? Thanks!

(Applying the patch without the CodeGenPGO change causes Profile/cxx-templates.cpp and CoverageMapping/branch-templates.cpp to fail as when mapRegionCounters() gets an instantiated template function it skips traversing the body)

[hokein]

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp

168

What does the Head mean?

209

There are many declaration in the testcode, hard to figure out which one we are chosen, can you add a comment about which declaration is chosen as the traversal root? I think it is function decl instantiated by the template int foo<4>();?

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

2107

moving this part of code to if (VisitBody) seems like a different fix (and I think it is reasonable, from the grammar, the ctor-initializer is part of the function-body), can we separate it and add a test?

[sammccall]

clang/include/clang/AST/RecursiveASTVisitor.h

2107

I can do it as a separate patch before if you like, but it's NFC.

Behavior only differs if there is an init list and VisitBody is false.
However !VisitBody requires one of:

• this is a declaration only => no init list
• this is a defaulted function => no init list

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp

168

Within the function

int foo<4>() { return 0; }.

I've called int foo<4>() the "head" and {return 0;} the "body".
The distinction is that the head was written, and the body was instantiated.

are there better terms? (I'd probably like this "head" better than "declaration part" so maybe we should just define these terms in the shouldVisitTemplateInstantiations() comment)

[davidxl]

clang/lib/CodeGen/CodeGenPGO.cpp

157

This one line change looks ok to me.