[PATCH] D58239: [clangd] Cache include fixes for diagnostics caused by the same unresolved name or incomplete type.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/IncludeFixer.cpp:130
+  // FIXME: consider batching the requests for all diagnostics.
   llvm::Optional<Symbol> Matched;
   Index.lookup(Req, [&](const Symbol &Sym) {
----------------
oops - this seems broken (in both the old and new form). Symbol is a shallow reference, and it's not valid after lookup() returns.

We can fix here or separately... one option to fix is the ResolvedSymbol struct suggested above.

(The fuzzyFind case looks ok)


================
Comment at: clangd/IncludeFixer.h:85
+  // name or incomplete type in one parse, especially when code is
+  // copy-and-pasted without #includes. As fixes are purely dependent on index
+  // requests and index results at this point, we can cache fixes by index
----------------
This seems pretty messy (the assumption that Fix depends only on the index lookup).
It saves some code now at the expense of being magic. If we want to e.g. insert qualifiers too, I worry it's going to (stop us || return incorrect cached results || lead to unneccesary cache misses) depending on what we do.

What would we need to store to calculate Fix?
Maybe a struct ResolvedSymbol with {scope, name, inserted header}? (or even the edit to insert the header)
If it's not hugely complicated, that seems both cleaner and more extensible - wdyt?


================
Comment at: clangd/IncludeFixer.h:90
+  mutable llvm::StringMap<std::vector<Fix>> FuzzyReqToFixesCache;
+  mutable llvm::StringMap<std::vector<Fix>> LookupIDToFixesCache;
 };
----------------
DenseMap<SymbolID, ...>?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58239