[PATCH] D120498: [AST] Test RecursiveASTVisitor's current traversal of templates.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp:12
+//
+// shouldVisitTemplateInstantiations() controls traversal of AST nodes that
+// were created by instantiation, rather than by written code. This means:
----------------
hokein wrote:
> I think this is a very useful documentation about the RAV API, instead of staying in the test file, I would suggest moving it to the RAV.h file.
Good idea. I suppose it belongs on shouldVisitTemplateInstantiations().


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp:18
+//   - Explicit instantiations are a tricky case:
+//     - the "declaration part" (template args, function params etc) was written
+//       and should always be traversed.
----------------
hokein wrote:
> Conceptually, an instantiations create a specific {Class, Var}TemplateSpecializationDecl, functionDecl.
> 
> ```
> template <class T> void foo(T) { }
> template void foo<char>(); // Line 2
> ```
> 
> For the statement on line 2 (called `explicit instantiation definition` I think)
> - there is no corresponding node in the clang AST, this is a missing feature in clang
> - it creates a FunctionDecl, that is `void foo(char) { }`,
> 
> does the "declaration part" in the comment refer to the declaration part of the `explicit instantiation definition` (I think so), or the declaration part of the FunctionDecl `void foo(char) {}`? The same question to the definition, my understanding from the comment is the function body of the `void foo(char) {}`, which is `{}`.
> 
> We describe the expected behavior, but there are some bugs or unimplemented features in clang, which creates discrepancies. I think this is confusing and hard to follow, not sure how to do here (encoding a bunch of FIXMEs here is not good,  not mentioning them at all is not good neither, can we mention only critical ones?)
> 
> 
> 
> 
> Conceptually, an instantiations create a specific {Class, Var}TemplateSpecializationDecl, functionDecl.
> 
> ```
> template <class T> void foo(T) { }
> template void foo<char>(); // Line 2
> ```
> 
> For the statement on line 2 (called `explicit instantiation definition` I think)
> - there is no corresponding node in the clang AST, this is a missing feature in clang
> - it creates a FunctionDecl, that is `void foo(char) { }`,

This is a plausible explanation (for functions and variables, but not classes). I don't think it's the best model - I'd prefer to say that the instantiated decl *corresponds* to the explicit instantiation definition.

In this example there are three decls: the template, the templated decl, and the instantiation.
For functions these are {FunctionTemplateDecl, FunctionDecl, FunctionDecl}.
For variables these are {VarTemplateDecl, VarDecl, VarTemplateSpecializationDecl}
For classes these are {ClassTemplateDecl, CXXRecordDecl, ClassTemplateSpecializationDecl}.
(Sometimes in -dump-ast they appear multiple times, but there are only 3 distinct pointers).

There are inconsistencies here:
 - function specializations don't have a distinct node type. I'm not sure this is terribly significant. (I suspect it's related to the fact they can't be partially specialized).
 - class specializations have locations that point to the explicit instantiation where possible (primary location, template parameter locations etc), and the template body otherwise. Variable and function specializations point at the template always.
 - in `-dump-ast` output it's inconsistent whether specializations are shown at the top level, under the template, or both. But I don't think there's any significant meaning behind this.

The second inconsistency (locations) is the closest to answering the question "do they correspond"?
Possible answers:
 - specializations should correspond to the explicit instantiations, so function + var are deficient in not reporting the locations
 - specializations should not correspond, and class specializations are buggy in reporting explicit instantiation locations
 - class specializations should correspond to the explicit instantiations, but others not (this seems silly)

I think #1 is the most plausible here, especially given that the specializations do know (getTemplateSpecializationKind()) if they were produced from an explicit instantiation.


> does the "declaration part" in the comment refer to the declaration part of the `explicit instantiation definition` (I think so), or the declaration part of the FunctionDecl `void foo(char) {}`? The same question to the definition, my understanding from the comment is the function body of the `void foo(char) {}`, which is `{}`.

This comment is talking about attributing AST nodes to source code, so it's talking about the FunctionDecl. By "declaration part" I mean the FunctionTypeLoc etc. I'll clarify this.

> We describe the expected behavior, but there are some bugs or unimplemented features in clang, which creates discrepancies. I think this is confusing and hard to follow, not sure how to do here (encoding a bunch of FIXMEs here is not good,  not mentioning them at all is not good neither, can we mention only critical ones?)

I think we should say "there are some bugs, see FIXMEs below".
The concepts are complicated and I think trying to mix them in with the implementation limitations will make a mess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120498/new/

https://reviews.llvm.org/D120498