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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added inline comments.


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


================
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 "
----------------
hokein wrote:
> 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.
Thank you for the extended explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139409