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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks good now, only blocker is dropping the change to LookupFile somehow.

Is it valuable to land this before the imminent 14 branch cut? I think it's OK but as an indexing change a crash has the possibility to be really disruptive, and most of the rest of us aren't regularly dogfooding this on Mac.
I'd suggest we only land it before the branch if you feel like you can test this on a reasonable variety of code in the next few weeks. WDYT?



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:293
+                               HeaderSearch &HS,
+                               FrameworkHeaderPath &HeaderPath) {
+    auto Res = CacheFrameworkToUmbrellaHeaderSpelling.try_emplace(Framework);
----------------
I'd suggest only passing the CharacteristicKind HeadersDir here rather than the whole FileEntry + HeaderPath.
Or failing that, being very explicit that these are just for an *example* header.

(The confusing part/trap here is that we're computing something based on one input, and then caching it so that it will be used for other inputs, without verifying that they would produce the same answer)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:309
+    if (StatErr) {
+      *CachedSpelling = "";
+      return llvm::None;
----------------
nit: this is a no-op


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:326
+    auto Path = FE->getName();
+    auto It = CachePathToFrameworkSpelling.find(Path);
+    if (It != CachePathToFrameworkSpelling.end())
----------------
any reason not to also try_emplace here and reuse the iterator below?


================
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] = "";
----------------
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...


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:712
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
----------------
Add a test that private headers are not replaced by the umbrella?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117056