kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks!
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:65
+ llvm::ArrayRef<SymbolReference> MacroRefs,
+ const Includes &, const PragmaIncludes *PI,
+ const SourceManager &SM, HeaderSearch &HS);
----------------
nit: name the `Includes` parameter. preferably `MainFileIncludes` (or
`ProvidedIncludes`)?
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:77
+ bool Satisfied = false;
+ for (const Header &H : Providers) {
+ if (H.kind() == Header::Physical && H.physical() == MainFile)
----------------
nit: maybe leave a comment here for skipping `Header`s we've seen before.
there's a quite good chance that we'll see same provider showing up multiple
times, skipping processing might be helpful (later on we'll probably need to
cache the analysis results for diagnostics purposes).
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:111
+ if (auto Edit = HI.insert(Insert.trim("<>\""), Insert.starts_with("<"))) {
+ if (PendingInsert && Edit->getOffset() == PendingInsert->getOffset()) {
+ PendingInsert = tooling::Replacement(
----------------
this logic makes me feel a little bit uneasy, as we're relying on alphabetical
sorting of `Results.Missing`, which isn't just the filenames but also contains
`<` or `"` at the beginning.
clang-format has weird include categories but I think it never mixes `"`
includes with `<` includes. so we're probably safe here.
but it might still be safer to just keep a map of offsets to edits, up to you.
(having a comment at the very least would be nice)
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:148
+)cpp";
+ Inputs.ExtraFiles["a.h"] = R"cpp(
+ #pragma once
----------------
nit: you can call guard(Code) here and elsewhere
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139013/new/
https://reviews.llvm.org/D139013
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
