[PATCH] D140095: [include-cleaner] Fix the member-expr-access usage for sugar type.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:50
+      return UT->getFoundDecl();
+    if (const auto *TST = Type->getAs<TemplateSpecializationType>())
+      return resolveTemplateName(TST->getTemplateName());
----------------
Resolving a template type to only its template name (ignoring template args) seems dubious in general. `Foo` is an important part of `vector<Foo>` and a critical part of `unique_ptr<Foo>`.

Currently we're only using this for the base types of MemberExpr, and here what we really care about is: which decl is responsible for this member? For this, the template (as opposed to args) is a reasonable heuristic. The exceptions I can think of:

 - dependent base: `template <class T> class Wrapper : public T {};` - this is rare I think
 - type aliases: `template <class T> using raw_pointer = T*;`
 - smart pointers (non-exception): `unique_ptr<T>`: the base of the member expr ends up being `T*` at least in non-dependent cases, so we're fine

So I'd suggest:
 - renaming this function to `getMemberProvider(QualType Base)` or so
 - adding a comment explicitly stating that the template name rather than params generally provide the members


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:66
     // A member expr implies a usage of the class type
     // (e.g., to prevent inserting a header of base class when using base
     // members from a derived object).
----------------
this comment is confusing: it suggests that reporting a usage preverts inserting a header!
Would be nice to clarify:
 - we report a usage so that code `returnFoo().bar` can keep `#include "foo.h"`
 - reporting the member decl would cause problems (inheritance, aliases)


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:70
     QualType Type = E->getBase()->IgnoreImpCasts()->getType();
     report(E->getMemberLoc(), resolveType(Type));
     return true;
----------------
only tangentially related, but should these be implicit references?

I don't think we actually want to insert headers based on them, right? Just allow people to keep the ones that they have inserted that are required for compilation?

This would also make it less severe if our heuristic above goes wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140095