sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:250
if (!MFI.HeaderID) {
elog("File {0} not found.", MFI.Written);
continue;
----------------
(sorry, I missed this before)
We should not be elogging this case, we're already emitting a diagnostic and it
doesn't indicate anything wrong with clangd itself.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:256
+ if (!Used && !mayConsiderUnused(MFI, AST)) {
+ dlog("{0} is not a valid unused header and will be filtered out",
+ MFI.Written);
----------------
This seems pretty vague text (particularly: what does valid mean, and is it
valid but used or invalid but unused). Maybe "{0} was not used, but is not
eligible to be diagnosed as unused"?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:64
/// Retrieves headers that are referenced from the main file but not used.
+/// System headers and inclusions of headers with no header guard are dropped.
std::vector<const Inclusion *>
----------------
I'm slightly worried about these sorts of comments becoming stale, they're
really just describing what we think "not used" means.
I'd replace it with something more general like "In unclear cases, headers are
not marked as unused", but up to you
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112695/new/
https://reviews.llvm.org/D112695
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
