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

[Sam McCall via Phabricator via cfe-commits]

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