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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
