[PATCH] D136293: [IncludeCleaner] Add public API

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
would be nice to have some high-level descriptions in the file comment.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:29
+  // FIXME: Add support for macros.
+  std::variant<Decl *, tooling::stdlib::Symbol> Storage;
+};
----------------
I see we use `Decl*` here and elsewhere (`WalkAST` etc), is there any reason of not using `const Decl*`?


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:48
+/// FIXME: Provide signals about the reference type and providing headers.
+using UsedSymbolVisitor = llvm::function_ref<void(
+    SourceLocation RefLoc, Symbol Target, llvm::ArrayRef<Header> Providers)>;
----------------
nit: rename the Visitor to CB? Visitor reminds me too much of `ASTVisitor`...


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:60
 
-} // namespace include_cleaner
-} // namespace clang
+void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, UsedSymbolVisitor CB) {
+  tooling::stdlib::Recognizer Recognizer;
----------------
sammccall wrote:
> IMO this function really deserves its own impl file I think (at least Analysis.cpp) - I think it + walkAST are going to be the two biggest bundles of complexity in the library. (if so, same with the tests)
+1


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:71
+      // FIXME: Handle IWYU pragmas, non self-contained files.
+      llvm::errs() << "Returning fileentry\n";
+      if (auto *FE = SM.getFileEntryForID(SM.getFileID(ND.getLocation())))
----------------
nit: remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136293