[PATCH] D138200: [include-cleaner] Make use of locateSymbol in WalkUsed

Mon Dec 5 23:13:13 PST 2022

kadircet added inline comments.

================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:44
+
+  // Cache for decl to header mappings, as the same decl might be referenced in
+  // multiple locations and finding providers for each location is expensive.
----------------
hokein wrote:
> I agree the place here might be on a performance-critical path, but I think the idea of using cache probably needs some bits of design, the current implementation seems half baked (there is also a FIXME in the findAllHeaders saying we should implement another cache for it, so it is unclear to me what's the whole picture).
> 
> Can we just leave out all cache in this patch with a FIXME? And having a follow-up patch for that?
i agree that the cache for `findHeaders` is a bit more nuanced, as it'll need to have assumptions about a given `SymbolLocation`, no matter how/where it was acquired will return the same results.
but i am not sure what's complicated/unclear about the cache for decl to headers. The benefit is clear as explained in the comment around saving computations every time a `foo` is referenced inside the file.
Since both the benefit is clear and the assumption around "a `decl` having the same set of providers" sounds simple and sane enough, i'd actually keep this one in.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138200