[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+      Config::UnusedIncludesPolicy::Experiment)
+    Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine
----------------
kadircet wrote:
> hokein wrote:
> > kadircet wrote:
> > > why do we need to collect pragmas in main file? i think we already have necessary information available via `IncludeStructure` (it stores keeps and exports, and we don't care about anything else in the main file AFAICT). so let's just use the PragmaIncludes we're getting from the Preamble directly? without even making a copy and returning a reference from the `Preamble` instead in `ParsedAST::getPragmaIncludes`
> > > i think we already have necessary information available via IncludeStructure (it stores keeps and exports, and we don't care about anything else in the main file AFAICT)
> > 
> > The IncludeStructure doesn't have a full support for IWYU export pragma, it only tracks the headers that have the export pragma.
> > 
> > My understand of the end goal is to use the `PragmaInclude` to handle every IWYU-related things, and we can remove all these IWYU bits in the `IncludeStructure`,  clangd IWYU pragma handling [code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).
> > The IncludeStructure doesn't have a full support for IWYU export pragma, it only tracks the headers that have the export pragma.
> 
> And I think that's all we need for IWYU pragmas inside the main file (as main file is a leaf and exporting headers from it doesn't change anything apart from making sure we keep them around)
> 
> > My understand of the end goal is to use the PragmaInclude to handle every IWYU-related things, and we can remove all these IWYU bits in the IncludeStructure, clangd IWYU pragma handling code.
> 
> Yes, I agree with some version of that, but until then this is introducing some extra changes (+ copying around more information in every AST build). So can we leave this piece out of the initial patch and just use the PragmaInclude we have from Preamble without copying it around?
> Yes, I agree with some version of that, but until then this is introducing some extra changes (+ copying around more information in every AST build). So can we leave this piece out of the initial patch and just use the PragmaInclude we have from Preamble without copying it around?

Sure, that sounds a good plan, and have a better narrowed scope of the patch. 


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:345
   auto Unused = computeUnusedIncludes(AST);
   EXPECT_THAT(Unused, ElementsAre(Pointee(writtenInclusion("<queue>"))));
+  EXPECT_THAT(computeUnusedIncludesNew(AST),
----------------
kadircet wrote:
> can you inline the call to `computeUnusedIncludes` into the EXPECT statement here?
sure, since this is not directly related to this patch, addressed in ccb67491f0dd55c5bd8ed5f71cb802422bfaa969.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140875/new/

https://reviews.llvm.org/D140875