[PATCH] D148112: [include-cleaner] Improve handling for templates

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:91
+    auto *CTSD = llvm::cast<ClassTemplateSpecializationDecl>(TD);
+    if (CTSD->isExplicitInstantiationOrSpecialization())
+      return CTSD;
----------------
We could consider the other order:

```
if (auto *Pattern = TD->getTemplateInstantiationPattern())
  return Pattern;
return TD;
```

To me this feels a bit more like there's an obvious principle and less like case bashing.

Concretely I think the difference is that we now report the pattern rather than the specialization for explicit template instantiations. As discussed offline, I'm not very familiar with the patterns where explicit instantiations are used. But it seems at least ambiguous whether a visible explicit instantiation is semantically significant, whereas it's clear that the pattern is important. So I think preferring the pattern here would be slightly better too, up to you.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:194
+  // Report a reference from explicit specializations/instantiations to the
+  // specialized template.
+  bool
----------------
maybe add to the comment that implicit ones are never visited?
(It should have been obvious to me, but it's easy to fall into the trap of "what are all the things a CTSD can be again?")


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:109
     ADD_FAILURE() << DiagBuf << "\nfrom code:\n" << ReferencingCode;
+  llvm::sort(TargetDeclKinds);
+  TargetDeclKinds.erase(llvm::unique(TargetDeclKinds), TargetDeclKinds.end());
----------------
this seems a little ad-hoc (is class name always enough info, why are we sorting/uniquing here)

Consider returning vector<NamedDecl*> and using a matcher to verify the things you care about (`kind(Decl::ClassTemplate)` or so?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148112