kadircet added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:54
Header(tooling::stdlib::Header H) : Storage(H) {}
+ Header(llvm::StringRef VerbatimSpelling) : Storage(VerbatimSpelling.str()) {}
----------------
can you add comments here describing what this case is used for
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:63
+// FIXME: use a real Class!
+using SymbolLocation = std::variant<SourceLocation, tooling::stdlib::Symbol>;
+
----------------
let's move this to `AnalysisInternal.h`, as it isn't used in any of the public
apis.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:45
+ llvm::SmallVector<Header> Results;
+ if (SLoc.index() == 0) {
+ // FIXME: Handle non self-contained files.
----------------
nit: `if (auto *Loc = std::get_if<SourceLocation>(&SLoc))`
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:55
+ if (!VerbatimSpelling.empty())
+ return {{VerbatimSpelling}};
+
----------------
what about exporters here?
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:58
+ Results = {{FE}};
+ // FIXME: compute a closure of exporter headers.
+ for (const auto *Export : PI.getExporters(FE, SM.getFileManager()))
----------------
not sure i follow the fixme here, are you suggesting we should also compute
transitive exporters?
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:63
+ }
+ if (SLoc.index() == 1) {
+ for (const auto &H : std::get<tooling::stdlib::Symbol>(SLoc).headers())
----------------
nit: `if (auto *Sym = std::get<tooling::stdlib::Symbol>(&SLoc))`
================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:49
+// FIXME: expose signals
+llvm::SmallVector<Header> findIncludeHeaders(const SymbolLocation &Loc,
+ const SourceManager &SM,
----------------
this is fine for the initial version, but we'll likely lose a lot of
performance here going forward. we can address it later on though, as this is
an internal api and those performance concerns won't be a problem until this is
used more widely.
we're likely going to call this function multiple times for each symbol, and
also despite having different sourcelocations, there'll probably be lots of
declarations that're originating from the same FileID.
Hence we'll want to accumulate outputs on a single container, rather than
returning a new container at each call, and also have some sort of file-id
based cache.
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:116
+
+ EXPECT_THAT(walk(AST),
+ UnorderedElementsAre(
----------------
can we use `findIncludeHeaders` directly?
that way we don't need to set up a referencing file, e.g. you can directly
create a SourceLocation inside `private.h` (in this case even the decl is not
important).
(same for the export test)
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:135
+ )cpp";
+ Inputs.ExtraFiles["private.h"] = R"cpp(class Private {};)cpp";
+ TestAST AST(Inputs);
----------------
calling this private is somewhat confusing, maybe call it `Detail`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137320/new/
https://reviews.llvm.org/D137320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
