[PATCH] D143319: [clangd] Support iwyu pragma: no_include

[Haojian Wu via Phabricator via cfe-commits]

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