sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:191
-// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
-// Standard Library headers are typically umbrella headers, and system headers
-// are likely to be the Standard Library headers. Until we have a good support
-// for umbrella headers and Standard Library headers, don't warn about them.
-bool mayConsiderUnused(const Inclusion *Inc) {
- return Inc->Written.front() != '<';
+bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes,
+ FileManager &FM, HeaderSearch &HeaderInfo) {
----------------
this is a local helper only, just pass in ParsedAST instead of all the extra
params?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:247
for (const Inclusion &MFI : Structure.MainFileIncludes) {
// FIXME: Skip includes that are not self-contained.
if (!MFI.HeaderID) {
----------------
this is fixed now (though I think it would be better addressed ~here)
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:292
auto Refs = findReferencedLocations(AST);
auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM),
AST.getIncludeStructure(), SM);
----------------
while here, pull out findReferencedFiles into its own var, and leave a FIXME to
adjust the set of referenced files to account for non-self-contained headers
whose symbols can be attributed to their include parents
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:311
for (const auto *Inc : computeUnusedIncludes(AST)) {
- if (!mayConsiderUnused(Inc))
+ if (!mayConsiderUnused(Inc, AST.getIncludeStructure(),
+ AST.getSourceManager().getFileManager(),
----------------
Oops, I missed this in previous review.
Why are we first computing which includes are unused (including ineligible) and
then filtering some out, rather than doing this check in the loop in
computeUnused?
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:1483
TEST(DiagnosticsTest, IncludeCleaner) {
Annotations Test(R"cpp(
----------------
This can be now or later, but we can't/shouldn't jam every feature into one
test case.
At some point we need to find a way to split this up into different cases, or
at least not have many features that can only be tested at the diagnostic level.
(If we call mayConsiderUnused inside computeUnused as suggested above, this no
longer has to be a diagnostic test)
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
