sammccall added a comment.
Mostly comment nits and was ready to approve, but I think I found a bug (getAs)
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:46
+ Base = Base->getPointeeType();
+ if (const auto *TT = Base->getAs<TypedefType>())
+ return TT->getDecl();
----------------
hmm, isn't `getAs<>` wrong and `dyn_cast` right here?
e.g. if we have a UsingType wrapping a TypedefType, we'll return the typedef
rather than the using (because we check for typedef first, and getAs<> will
unwrap)
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:50
+ return UT->getFoundDecl();
+ // A heuristic to resolve a template type to **only** its template name.
+ // We're only use this method for the base type of MemberExpr, in general
----------------
nit: "A heuristic to" => "A heuristic: "
(otherwise it sounds like the *aim* here that we're approximating is to resolve
to only the template name, rather than resolving to the template name being the
approximation)
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:51
+ // A heuristic to resolve a template type to **only** its template name.
+ // We're only use this method for the base type of MemberExpr, in general
+ // the template name provides the member, and the critial case
----------------
use -> using
critial -> critical
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:52
+ // We're only use this method for the base type of MemberExpr, in general
+ // the template name provides the member, and the critial case
+ // `unique_ptr<Foo>` is handled (the base type is a Foo*).
----------------
the template name -> the template
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:72
bool VisitMemberExpr(MemberExpr *E) {
- // 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).
+ // Reporting a usage of the member decl will cause issues (when the base
+ // type is a type alias or a subclass). Instead, we report a usage of the
----------------
will => would (subjunctive makes it clearer this is *not* what we're doing)
cause issues => say what the problems are: "(e.g. force including the base
class for inherited members)"
(sorry for unclear previous comment)
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
