kadircet added a comment.
regarding testing, i think updating all tests that we have in
`IncludeCleanerTests` that call `computeUnusedIncludes` to also call the new
function that'll use include-cleaner library should be enough.
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:5
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../include-cleaner/include)
+
----------------
can you move this near the include_directories for clang-tidy below (near line
63)
================
Comment at: clang-tools-extra/clangd/Config.h:91
- enum UnusedIncludesPolicy { Strict, None };
+ enum UnusedIncludesPolicy { Strict, None, Experiment };
/// Controls warnings and errors when parsing code.
----------------
can you also add a comment saying that "`Experiment` tries to provide the same
behaviour as `Strict` but using include-cleaner"
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+
+ // FIXME: this map should probably be in IncludeStructure.
+ llvm::StringMap<llvm::SmallVector<IncludeStructure::HeaderID>> BySpelling;
----------------
agreed, let's change `StdlibHeaders` in `IncludeStructure` to actually be a
`BySpelling` mapping, it should be no-op for existing implementation as we're
only checking for stdlib headers already (and there's no other usage)
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+ }
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector<include_cleaner::SymbolReference> Macros;
----------------
this might behave slightly different than what we have in `RecordedPP`, and
rest of the applications of include-cleaner will be using that. can we expose
the pieces in RecordedPP that collects MacroRefs as a separate handler and
attach that collector (combined with the skipped range collection inside
`CollectMainFileMacros` and also still converting to `MainFileMacros` in the
end (as we can't store sourcelocations/identifierinfos from preamble)?
afterwards we can use the names stored in there to get back to
`IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:487
+ Macros.push_back({Tok.location(),
+ include_cleaner::Macro{/*Name=*/nullptr, DefLoc},
+ include_cleaner::RefType::Explicit});
----------------
you can get the `IdentifierInfo` using `Name` inside `Macro` and PP.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:512
+ });
+ std::vector<const Inclusion *> Unused;
+ for (const auto &I : Includes.MainFileIncludes) {
----------------
let's use `getUnused` directly here, with an empty set of `PublicHeaders`.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:539
const Config &Cfg = Config::current();
- if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict ||
+ if (Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::None ||
Cfg.Diagnostics.SuppressAll ||
----------------
can you keep `computeUnusedIncludes` the same and introduce a new function
that'll call `include_cleaner`? afterwards we can just do a switch on policy
here and call the relevant function.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+ Config::UnusedIncludesPolicy::Experiment)
+ Pragmas.record(*Clang);
// Copy over the macros in the preamble region of the main file, and combine
----------------
why do we need to collect pragmas in main file? i think we already have
necessary information available via `IncludeStructure` (it stores keeps and
exports, and we don't care about anything else in the main file AFAICT). so
let's just use the PragmaIncludes we're getting from the Preamble directly?
without even making a copy and returning a reference from the `Preamble`
instead in `ParsedAST::getPragmaIncludes`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140875/new/
https://reviews.llvm.org/D140875
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
