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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
