hokein added a comment.
Thanks for the patch, and sorry for the long delay.
You probably need to do a large rebase when updating this patch (since the
include-cleaner clangd code has been changed a lot)
This is a big patch, touching several pieces (I'd suggest split this patch, and
narrow its scope.):
1. support no_include pragma in include-cleaner library (I made some comments
regarding the implementation)
2. usage of no_include pragma:
- 2.1 include-cleaner diagnostics in clangd -- for missing-includes, we
should not insert a spelled include when it is in the NoIncludeSet;
- 2.2 global code completion respects no_include pragma
It makes sense to support 1 and 2.1. And 2.2 is nice to have -- global code
completion was built long time ago, and we haven't heard of any user complains,
I'm less certain, it is probably fine if it is trivial to implement.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:579
+std::vector<Diag> issueNoIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code) {
----------------
I'm not sure it is worth to make a dedicated function for this marginal pragma,
I think we can treat it as an unused-include case, and we can move it to
`issueUnusedIncludesDiagnostics`.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:44
+struct IwyuNoInclude {
+ std::string
----------------
I think we can do it simpler. The `Resolved` field is no used anywhere, and is
expensive to get.
The critical information to store is the header spelled in the no_include
pragma, so having a `StringSet<> NoIncludes` field in `PragmaIncludes` should
be sufficient.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:165
+/// \class PragmaIncludes because the directive could be scattered throughout
+/// the source rather than being limited in preamble bounds.
+struct RecordedNoIncludes {
----------------
This is not a specific issue for no_include pragma, it is a known limitation --
ideally we should make PragmaIncludes collect all IWYU pragmas outside of the
preamble region, tracking in https://github.com/clangd/clangd/issues/1571.
I think we should merge this logic to the `PragmaIncludes` structure.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143319/new/
https://reviews.llvm.org/D143319
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
