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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
