[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:295
+    if (!R.second && !R.first->second.empty()) {
+      // Framework has known umbrella spelling, cache it for this header as
+      // well.
----------------
sammccall wrote:
> Why? Seems like just looking up the umbrella first would be simpler and just as efficient.
To give a proper spelling for private framework headers (although maybe the framework import would be more useful than a non-framework import? I dunno)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:303
+      SpellingR.first->second = "";
+      R.first->second = "";
+      return R.first->second;
----------------
sammccall wrote:
> if the header we happened to see first is PrivateHeaders, then you're caching the fact that this framework has no umbrella header.
> 
> There's a design decision here: is the path of any single header sufficient to decide whether a framework has an umbrella? If not, the cache needs to work differently.
Fixed


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:308
+
+    SmallString<256> UmbrellaPath(Info.HeadersDir);
+    llvm::sys::path::append(UmbrellaPath, Framework + ".h");
----------------
sammccall wrote:
> comment here on what an umbrella header is and why we care
Added a comment to the top of getFrameworkUmbrellaSpelling


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:762
+      Header, Main,
+      {"-iframeworkwithsysroot", FrameworksPath, "-xobjective-c++"});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
----------------
sammccall wrote:
> OOC, is "withsysroot" needed here?
Changed, should have been iframework to add a system framework path


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117056