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

David Goldman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 19 15:33:29 PST 2022

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.

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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list