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