[PATCH] D117056: [clangd] Properly compute framework-style include spelling

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:326
+    auto Path = FE->getName();
+    auto It = CachePathToFrameworkSpelling.find(Path);
+    if (It != CachePathToFrameworkSpelling.end())
----------------
sammccall wrote:
> any reason not to also try_emplace here and reuse the iterator below?
In case the splitFrameworkHeaderPath fails we shouldn't cache the failure. But it's unexpected and shouldn't happen in practice now that I've added the private headers support, so flipped it to remove the entry if it happens.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:334
+      // them. Cache the failure as well so we won't do this again for the same
+      // header.
+      CachePathToFrameworkSpelling[Path] = "";
----------------
sammccall wrote:
> We're (deliberately?) bypassing the umbrella cache here, say why private headers shouldn't be replaced by the umbrella header?
> Presumably because they're not exported by it...
Just went ahead and added proper support for privateheaders since it wasn't too much more work. They should have a separate umbrella header (or none), see https://clang.llvm.org/docs/Modules.html#private-module-map-files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117056