[PATCH] D108194: [clangd] IncludeCleaner: Mark used headers

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:158
+
+std::vector<Inclusion>
+getUnused(IncludeStructure::HeaderID EntryPoint,
----------------
sammccall wrote:
> sammccall wrote:
> > Why are we passing around Inclusions by value?
> Sorry, should have thought this through more before leaving the comment. There are a couple of questions really:
> 
> How should we *store* the information about which inclusions are unused?
>  - not at all, generate ReferencedFiles and compute "is this header unused" on the fly when generating diagnostics
>  - store ReferencedFiles but call "is this header unused" on the fly
>  - store a boolean or something in each Inclusion
>  - store a list of the inclusions that are unused
> IMO this is mostly a question of what's the lifecycle of the info, and what's the simplest representation - seems like we should prefer stuff higher on the list if we have a choice.
> 
> What should the signature of the function be?
> There doesn't seem to be any work saved here by processing all the includes as a batch - why not simplify by just making this `bool isUnused(...)` and let the caller/test choose what data structures to use?
OK I was confused, nothing is getting stored, but ParsedAST::getUnused() function creates/destroys the analysis data so it needs to run as a batch.

I think probably:
 - *this* function should just be a simple `bool isUnused(...)` and the loop lives in the caller
 - `ParsedAST::getUnused()` should become `getUnused(const ParsedAST&)` and live in this file

We have some circularity between ParsedAST and IncludeCleaner, but I think we're going to have that in any case due to `findReferencedLocations()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108194