[PATCH] D136293: [IncludeCleaner] Add public API

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! Looks good, feel free to address/disregard design comments as you see fit.



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:14
+#include "llvm/ADT/STLFunctionalExtras.h"
+#include <variant>
+
----------------
This is awful but: as mentioned on another patch, we can't actually use variant. AFAIK all C++17 *language* features are available, but LLVM supports apple clang 9.3, which doesn't provide <variant> at all.

There's an IntrusiveVariant in review somewhere but seems stalled (and honestly the Intrusive part makes it pretty ugly to use).

I think for now our best option is a Kind enum and some kind of uintptr_t storage :-(


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:32
+
+/// Represents a file that's providing some symbol. Might not be includeable.
+struct Header {
----------------
nit: that's providing -> that provides, includeable -> includable

A bit unclear "what might not be includeable" means, say why?


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:34
+struct Header {
+  Header(const FileEntry *FE) : Storage(FE) {}
+  Header(tooling::stdlib::Header H) : Storage(H) {}
----------------
a short comment for each case is pretty easy-to-read way to doc sum types, and these are important enough concepts it might be justified here. Up to you though.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:47
+/// that symbol may be provided by several headers.
+/// FIXME: Provide signals about the reference type and providing headers.
+using UsedSymbolVisitor = llvm::function_ref<void(
----------------
this is so the caller can filter the references and rank the results, right?

I think it's worth saying so, mostly because that's an important design decision, and I keep forgetting to write them down


================
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;
----------------
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)


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:66
+      if (auto SS = Recognizer(&ND)) {
+        llvm::errs() << "returning stdlib\n";
+        return CB(Loc, Symbol(*SS), toHeader(SS->headers()));
----------------
nit: remove debug


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:67
+        llvm::errs() << "returning stdlib\n";
+        return CB(Loc, Symbol(*SS), toHeader(SS->headers()));
+      }
----------------
(I think the patch is fine, but could use some extra docs/fixmes depending on what the choices are here).

In general we want a forward declaration in the main file to suppress any include insertion.
Two obvious ways to achieve that:
A) walkUsed doesn't report refs of symbols that have a decl in the main file
B) walkUsed reports the refs, the header list includes the main file, the caller recognizes it when checking if the ref is satisfied

A) feels more ad-hoc, but seems to work and *does* naturally achieve the nice effect that `#include "foo.h"` is marked as unused if the only used symbols are also forward-declared locally. In this case, maybe add a FIXME for this filtering? Also A seems surprising enough to be worth documenting.

B) falls more naturally out of the implementation. It looks like  we have a bug here: by bailing out early, we'll omit any forward declarations from the main file. For symbols in `namespace std` we're arguably [within our rights as such decls are UB](http://eel.is/c++draft/namespace.std#1). However such forward decls are legal in C, so maybe we should care after all? In any case, this subtlety deserves some sort of comment.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:72
+      llvm::errs() << "Returning fileentry\n";
+      if (auto *FE = SM.getFileEntryForID(SM.getFileID(ND.getLocation())))
+        return CB(Loc, Symbol(ND), {Header(FE)});
----------------
FIXME: handle locations from macros


================
Comment at: clang-tools-extra/include-cleaner/unittests/CMakeLists.txt:21
   clangFrontend
+  clangToolingInclusionsStdlib
   )
----------------
needed in the main lib too?


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:125
 
+TEST(WalkUsed, Basic) {
+  llvm::Annotations HeaderCode(R"cpp(
----------------
signal-to-noise in this test is a bit low, and there'll be a bunch more tests as the features get extended.

I think adding a `TEST_F` fixture and splitting the stdlib out into its own test, trying to maximize the shared code, may lead to cleaner tests.

Feel free to defer this, just watch out for incremental inertia giving you a 1000 line monster before you know it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136293