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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice!



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:197
+  llvm::StringMap<std::string> CachePathToFrameworkSpelling;
+  llvm::StringMap<std::string> CacheFrameworkToUmbrellaInclude;
 
----------------
...ToUmbrellaHeaderSpelling?

(Unclear to me what values "umbrella include" would take)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:260
+
+    bool valid() { return !HeadersDir.empty() && !HeaderSubpath.empty(); }
+  };
----------------
return `Optional<FrameworkInfo>` instead of sentinel values


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:263
+
+  FrameworkInfo getHeaderFrameworkInfo(llvm::StringRef Path) {
+    using namespace llvm::sys;
----------------
This name is very vague: `splitFrameworkHeaderPath`?
Similarly FrameworkInfo -> FrameworkHeaderPath.

(I was confused why this wasn't cached until I understood this *isn't* per-framework information, it's per-header information)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:285
+
+  llvm::StringRef getFrameworkIncludeSpelling(llvm::StringRef Path,
+                                              const FileEntry *FE,
----------------
Short doc comment here. In particular what `Framework` contains


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:285
+
+  llvm::StringRef getFrameworkIncludeSpelling(llvm::StringRef Path,
+                                              const FileEntry *FE,
----------------
sammccall wrote:
> Short doc comment here. In particular what `Framework` contains
no need to pass `Path`, it's FE->getName()


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:291
+    if (!SpellingR.second)
+      return SpellingR.first->second;
+
----------------
there are 7 references to SpellingR.first->second in this function, give it a name?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:293
+
+    auto R = CacheFrameworkToUmbrellaInclude.try_emplace(Framework);
+    if (!R.second && !R.first->second.empty()) {
----------------
Having multiple caches managed by this function makes it hard to understand.
Pull out a function to get the (cached) umbrella header for a framework?


================
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.
----------------
Why? Seems like just looking up the umbrella first would be simpler and just as efficient.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:303
+      SpellingR.first->second = "";
+      R.first->second = "";
+      return R.first->second;
----------------
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");
----------------
comment here on what an umbrella header is and why we care


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:351
     }
+    // Special case framework headers since they have special framework-style
+    // include spelling. We also check if an umbrella header with the same name
----------------
This comment is most useful to people not very familiar with frameworks, but isn't specific enough. maybe:
`Framework headers are spelled as <Framework/Foo.h>, not "path/Framework.framework/Headers/Foo.h"`

The bit about umbrella headers should go where we do that, and mostly say *why* rather than what


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:356
+    if (const auto *HFI = HS.getExistingFileInfo(FE, /*WantExternal*/ false)) {
+      auto Framework = HFI->Framework;
+      if (!Framework.empty()) {
----------------
nit: inline, this var doesn't improve readability


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:358
+      if (!Framework.empty()) {
+        auto Spelling =
+            getFrameworkIncludeSpelling(Filename, FE, Framework, HS);
----------------
return Optional<StringRef> and initialize this in the if, rather than using the empty string as a sentinel value


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:360
+            getFrameworkIncludeSpelling(Filename, FE, Framework, HS);
+        if (!Spelling.empty()) {
+          return Spelling;
----------------
nit: no braces


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:669
+  auto FrameworksPath = testPath("Frameworks/");
+  auto FrameworkHeaderPath =
+      testPath("Frameworks/Foundation.framework/Headers/NSObject.h");
----------------
nit: inline these filenames used once


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:671
+      testPath("Frameworks/Foundation.framework/Headers/NSObject.h");
+  const std::string FrameworkHeader = R"(
+    __attribute((objc_root_class))
----------------
nit: no const on locals


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:695
+
+TEST_F(SymbolCollectorTest, ObjCUmbrellaFrameworkIncludeHeader) {
+  CollectorOpts.CollectIncludePath = true;
----------------
Instead of copying the setup 3 times, can we:
 - run the test, expect "Foundation/NSObject.h"
 - add the umbrella file
 - run the test, expect "Foundation/Foundation.h"
 - run the test with -isysroot, expect <Foundation/Foundation.h>
?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:762
+      Header, Main,
+      {"-iframeworkwithsysroot", FrameworksPath, "-xobjective-c++"});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
----------------
OOC, is "withsysroot" needed here?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1018
   } TestCases[] = {
-    {
-      R"cpp(
+      {
+          R"cpp(
----------------
unrelated whitespace changes


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1052
         HFI.IndexHeaderMapHeader = 1;
+    } else if (CurDir->isFramework()) {
+      size_t SlashPos = Filename.find('/');
----------------
This looks like a separate change, and should have a test in clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117056