[PATCH] D135859: [Includecleaner] Introduce RefType to ast walking

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:33
+/// Indicates the relation between the reference and the target.
+enum class RefType {
+  /// Target is explicit from the reference, e.g. function call.
----------------
I'm wondering what's our plan of supporting policy (different coding-style may have different decisions about which includes are used)?

IIUC, the RefType is part of the picture, providing fine-grained information about each reference, and the caller can make their decisions based on it?

Thinking about the `Implicit` type, I think cases like non-spelled constructor call, implicit conversion calls (maybe more?) fall into this type, if we support `Policy.Constructor`, and `Policy.Conversion`, how do we distinguish with these cases? We can probably do some ad-hoc checks on the `TargetDecl`, but I'm not sure that the tuple `<SourceLocation, TargetDecl, Ref>` will provide enough information to implement different policy .


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:36
+  Explicit,
+  /// Target isn't spelled, e.g. default constructor call.
+  Implicit,
----------------
nit: `default constructor call` is vague -- `S s = S();` it is a default constructor call, but it is not implicit, maybe more specific by giving a simple example, like default constructor call in `S s;`?


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:12
 #include "gtest/gtest.h"
+#include <map>
+#include <unordered_map>
----------------
nit: it seems unused.


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:177
 TEST(WalkAST, ConstructExprs) {
-  testWalk("struct ^S {};", "S ^t;");
-  testWalk("struct S { ^S(int); };", "S ^t(42);");
+  testWalk("struct $implicit^S {};", "S ^t;");
+  testWalk("struct S { $explicit^S(int); };", "S ^t(42);");
----------------
maybe add this following case, I think it will mark the the constructor `S()` implicit. 

```
struct S { S(); };
S ^t;
```


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:178
+  testWalk("struct $implicit^S {};", "S ^t;");
+  testWalk("struct S { $explicit^S(int); };", "S ^t(42);");
 }
----------------
nit: also  add an implicit case

```
S t = 42; 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135859