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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
