sammccall added a comment.
This is cool!
I think it will interact with other features (stdlib, exported headers), and so
whatever lands second is going to have to eat the complexity of that
interaction.
I think both stdlib and exported headers are more critical and more essentially
complex, so should probably land first to drive the design.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:298
+
+llvm::StringSet<> reachableUsedHeaders(
+ const IncludeStructure &IncludeGraph, IncludeStructure::HeaderID Source,
----------------
It seems like this is going to include any files that are transitively
reachable through multiple paths and directly reachable through none. These are
going to seem like false positives.
Like <stddef.h> if the file uses size_t at all, or something.
Sure, if you want the whole file to be IWYU-clean you should remove <unused.h>
and add <stddef.h>, but they're not *really* related.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:311
+ Result.insert(IncludeGraph.getRealPath(NextID));
+ for (const auto &Header : IncludeGraph.IncludeChildren.lookup(NextID))
+ if (!Visited.contains(Header))
----------------
This is going to have problems with private/exported headers.
Consider includes main -> foo -> bar -> private.
Main uses a symbol from private, bar exports private.
You need to suggest replacing the include of foo -> {bar}, not {bar, private}.
However you still want to traverse children of private in case main uses those.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:376
D.Fixes.back().Edits.emplace_back();
- D.Fixes.back().Edits.back().range.start.line = Inc->HashLine;
- D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1;
+ D.Fixes.back().Edits.back().range.start.line = I.Include->HashLine;
+ D.Fixes.back().Edits.back().range.end.line = I.Include->HashLine + 1;
----------------
We should offer both fixes as alternatives, maybe?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:381
+ DirectIncludes +=
+ llvm::formatv("#include \"{0}\"\n", Replacement.getKey());
+ }
----------------
exactly how to spell an include and where to place it is tricky. See
IncludeInserter.h in headers.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:69
-std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+llvm::StringSet<> reachableUsedHeaders(
+ IncludeStructure &IncludeGraph, IncludeStructure::HeaderID Source,
----------------
why are we back to passing around strings?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113262/new/
https://reviews.llvm.org/D113262
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
