sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/Headers.h:65
SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
+ unsigned ID = std::numeric_limits<unsigned>::max(); // Corresponding
HeaderID.
};
----------------
I don't think we're under any size pressure here - `Optional<unsigned>`?
================
Comment at: clang-tools-extra/clangd/Headers.h:65
SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
+ unsigned ID = std::numeric_limits<unsigned>::max(); // Corresponding
HeaderID.
};
----------------
sammccall wrote:
> I don't think we're under any size pressure here - `Optional<unsigned>`?
call the member HeaderID, rather than have this as a comment only?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:161
+ const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
+ const SourceManager &SM) {
+ std::vector<const Inclusion *> Unused;
----------------
SM is unused
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:165
+ // FIXME: Skip includes that are not self-contained.
+ if (MFI.Resolved.empty()) {
+ elog("{0} is unresolved", MFI.Written);
----------------
this can just be a check whether MFI.ID is valid or not
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:171
+ assert(IncludeID != IncludeStructure::HeaderID::Invalid);
+ bool Used = ReferencedFiles.find(IncludeID) != ReferencedFiles.end();
+ if (!Used) {
----------------
ReferencedFiles.contains(IncludeID) and inline into the if?
================
Comment at: clang-tools-extra/clangd/ParsedAST.h:162
+std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+
----------------
I think this function belongs in IncludeCleaner.h.
(There's a circularity problem between ParsedAST and IncludeCleaner, but
putting the function here doesn't fix it)
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:165
+ for (const auto &Include : UnusedIncludes) {
+ // Strip enclosing "".
+ UnusedHeaders.push_back(
----------------
Don't bother doing this stripping just for the test IMO, it obscures the
assertion (more than escaping quotes would)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108194/new/
https://reviews.llvm.org/D108194
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
