[PATCH] D139409: [include-cleaner] Handle dependent type members in AST

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:67
 
+  bool VisitCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr *E) {
+    QualType QT = E->getBaseType().getCanonicalType();
----------------
Rebase the current patch. Move the implementation in the `resolveType` method, then this method will just pass the `BaseType` to `resolveType`.



================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:79
+
+    if (isa<TemplateSpecializationType>(UnqualifiedType)) {
+      const TemplateSpecializationType *TST =
----------------
if we just aim to support the `vector<T>().size()` case, we only need this part of code right?


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:81
+      const TemplateSpecializationType *TST =
+          cast<TemplateSpecializationType>(UnqualifiedType);
+      TemplateDecl *TD = TST->getTemplateName().getAsTemplateDecl();
----------------
You can simplify it in a single if statement:

`if (const TemplateSpecializationType *TS = Type->getAs<TemplateSpecializationType>()) `


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:198
+           "template<typename T> void k(typename Base<T>::Nested d) { "
+           "d.^method(); }");
+  testWalk("template<typename T> struct $explicit^Base { struct Nested { void "
----------------
Thanks for coming up these cases. IMO a more reasonable behavior would be that the target symbol of the reported reference is `Nested` rather than the outer class `Base`, but implementing it would require some dependent name lookup in the `Base` (in clangd we have a similar thing `HeuristicResolver`, but it is sophisticated).

I think it is find to ignore these cases (these cases are more tricky, and less interesting to us) right now, this would simplify the implementation.

Note that the existing behavior of accessing a member expr from a dependent type is that we don't report any reference on `d.^method()`, so the original "include base header via a base-member-access from a derived-class object" issue doesn't exist. We aim to improve this behavior for some critical cases.

The critical case we want to support is the `vector<T>().size()`, if other cases come up in the furture, we could revisit it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139409