[PATCH] D90763: Traverse-ignore explicit template instantiations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a tiny commenting request.



================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:506
+    if (const auto *CTSD = Node.get<ClassTemplateSpecializationDecl>()) {
+      auto SK = CTSD->getSpecializationKind();
+      if (SK == TSK_ExplicitInstantiationDeclaration ||
----------------
steveire wrote:
> aaron.ballman wrote:
> > Same here, though this could also be simplified to:
> > ```
> > ScopedTraversal = (SK == TSK_ExplicitInstantiationDeclaration || SK == TSK_ExplicitInstantiationDefinition);
> > ```
> If `ScopedTraversal` is set to `true` above, this could wrongly set it to `false`.
> If ScopedTraversal is set to true above, this could wrongly set it to false.

Good catch!


================
Comment at: clang/unittests/AST/ASTTraverserTest.cpp:1092
+
+// Explicit instantiation of template functions do not appear in the AST
+template float timesTwo(float);
----------------
steveire wrote:
> aaron.ballman wrote:
> > Huh, do you have any idea if that's a bug? We have `ClassTemplateSpecializationDecl` and `VarTemplateSpecializationDecl`, but we have `FunctionTemplateSpecializationInfo` that doesn't generate an AST node and no mention of why in the comments that I've spotted yet.
> I don't know why, but this is part of the confusion in the test in the discussion below.
> 
> I can look into it after this is merged if you don't beat me to it.
> I don't know why, but this is part of the confusion in the test in the discussion below.

Okay, I kind of thought that might be the case, thank you for confirming.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2280
+    EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+    EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
----------------
steveire wrote:
> aaron.ballman wrote:
> > Explicitly instantiating a function template in ignore mode returns false, but explicitly instantiating a class template returns true? Is this intentional or just fallout from the lack of explicit instantiation information in the AST for functions?
> I've added some more tests and comments to try to clarify this.
> 
> We should be able to match on the template arguments of explicit instantiations, but not the contents of the instantiations.
> 
> Lack of representation of explicit function instantiations makes the expected test results confusing, but hopefully the comments now clarify.
Thanks for the new comment, that clarifies nicely! Can you add a full stop to the end of the comment though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90763