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