diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -328,42 +328,33 @@ // instead of // which should be used instead of directly // importing the header. - std::optional getFrameworkUmbrellaSpelling( - llvm::StringRef Framework, SrcMgr::CharacteristicKind HeadersDirKind, - const HeaderSearch &HS, FrameworkHeaderPath &HeaderPath) { + std::optional + getFrameworkUmbrellaSpelling(llvm::StringRef Framework, + const HeaderSearch &HS, + FrameworkHeaderPath &HeaderPath) { auto Res = CacheFrameworkToUmbrellaHeaderSpelling.try_emplace(Framework); auto *CachedSpelling = &Res.first->second; if (!Res.second) { return HeaderPath.IsPrivateHeader ? CachedSpelling->PrivateHeader : CachedSpelling->PublicHeader; } - bool IsSystem = isSystem(HeadersDirKind); SmallString<256> UmbrellaPath(HeaderPath.HeadersParentDir); llvm::sys::path::append(UmbrellaPath, "Headers", Framework + ".h"); llvm::vfs::Status Status; auto StatErr = HS.getFileMgr().getNoncachedStatValue(UmbrellaPath, Status); - if (!StatErr) { - if (IsSystem) - CachedSpelling->PublicHeader = llvm::formatv("<{0}/{0}.h>", Framework); - else - CachedSpelling->PublicHeader = - llvm::formatv("\"{0}/{0}.h\"", Framework); - } + if (!StatErr) + CachedSpelling->PublicHeader = llvm::formatv("<{0}/{0}.h>", Framework); UmbrellaPath = HeaderPath.HeadersParentDir; llvm::sys::path::append(UmbrellaPath, "PrivateHeaders", Framework + "_Private.h"); StatErr = HS.getFileMgr().getNoncachedStatValue(UmbrellaPath, Status); - if (!StatErr) { - if (IsSystem) - CachedSpelling->PrivateHeader = - llvm::formatv("<{0}/{0}_Private.h>", Framework); - else - CachedSpelling->PrivateHeader = - llvm::formatv("\"{0}/{0}_Private.h\"", Framework); - } + if (!StatErr) + CachedSpelling->PrivateHeader = + llvm::formatv("<{0}/{0}_Private.h>", Framework); + return HeaderPath.IsPrivateHeader ? CachedSpelling->PrivateHeader : CachedSpelling->PublicHeader; } @@ -386,21 +377,14 @@ CachePathToFrameworkSpelling.erase(Res.first); return std::nullopt; } - auto DirKind = HS.getFileDirFlavor(FE); if (auto UmbrellaSpelling = - getFrameworkUmbrellaSpelling(Framework, DirKind, HS, *HeaderPath)) { + getFrameworkUmbrellaSpelling(Framework, HS, *HeaderPath)) { *CachedHeaderSpelling = *UmbrellaSpelling; return llvm::StringRef(*CachedHeaderSpelling); } - if (isSystem(DirKind)) - *CachedHeaderSpelling = - llvm::formatv("<{0}/{1}>", Framework, HeaderPath->HeaderSubpath) - .str(); - else - *CachedHeaderSpelling = - llvm::formatv("\"{0}/{1}\"", Framework, HeaderPath->HeaderSubpath) - .str(); + *CachedHeaderSpelling = + llvm::formatv("<{0}/{1}>", Framework, HeaderPath->HeaderSubpath).str(); return llvm::StringRef(*CachedHeaderSpelling); } diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp --- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp @@ -702,9 +702,9 @@ EXPECT_THAT( Symbols, UnorderedElementsAre( - AllOf(qName("NSObject"), includeHeader("\"Foundation/NSObject.h\"")), + AllOf(qName("NSObject"), includeHeader("")), AllOf(qName("PrivateClass"), - includeHeader("\"Foundation/NSObject+Private.h\"")), + includeHeader("")), AllOf(qName("Container")))); // After adding the umbrella headers, we should use that spelling instead. @@ -722,13 +722,13 @@ "Foundation_Private.h"), 0, llvm::MemoryBuffer::getMemBuffer(PrivateUmbrellaHeader)); runSymbolCollector(Header, Main, {"-F", FrameworksPath, "-xobjective-c++"}); - EXPECT_THAT(Symbols, - UnorderedElementsAre( - AllOf(qName("NSObject"), - includeHeader("\"Foundation/Foundation.h\"")), - AllOf(qName("PrivateClass"), - includeHeader("\"Foundation/Foundation_Private.h\"")), - AllOf(qName("Container")))); + EXPECT_THAT( + Symbols, + UnorderedElementsAre( + AllOf(qName("NSObject"), includeHeader("")), + AllOf(qName("PrivateClass"), + includeHeader("")), + AllOf(qName("Container")))); runSymbolCollector(Header, Main, {"-iframework", FrameworksPath, "-xobjective-c++"}); diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -2003,8 +2003,10 @@ } else if (DL.isFramework()) { StringRef Dir = DL.getFrameworkDirRef()->getName(); if (CheckDir(Dir)) { + // Framework includes by convention use <> so mark these as system + // so the caller know to use <> instead of "" as well. if (IsSystem) - *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic()); + *IsSystem = BestPrefixLength; BestPrefixIsFramework = true; } } diff --git a/clang/unittests/Lex/HeaderSearchTest.cpp b/clang/unittests/Lex/HeaderSearchTest.cpp --- a/clang/unittests/Lex/HeaderSearchTest.cpp +++ b/clang/unittests/Lex/HeaderSearchTest.cpp @@ -47,14 +47,18 @@ Search.AddSearchPath(DL, /*isAngled=*/false); } - void addSystemFrameworkSearchDir(llvm::StringRef Dir) { + void addFrameworkSearchDir(llvm::StringRef Dir, bool IsSystem = true) { VFS->addFile( Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/std::nullopt, /*Group=*/std::nullopt, llvm::sys::fs::file_type::directory_file); auto DE = FileMgr.getOptionalDirectoryRef(Dir); assert(DE); - auto DL = DirectoryLookup(*DE, SrcMgr::C_System, /*isFramework=*/true); - Search.AddSystemSearchPath(DL); + auto DL = DirectoryLookup(*DE, IsSystem ? SrcMgr::C_System : SrcMgr::C_User, + /*isFramework=*/true); + if (IsSystem) + Search.AddSystemSearchPath(DL); + else + Search.AddSearchPath(DL, /*isAngled=*/true); } void addHeaderMap(llvm::StringRef Filename, @@ -175,7 +179,7 @@ } TEST_F(HeaderSearchTest, SdkFramework) { - addSystemFrameworkSearchDir( + addFrameworkSearchDir( "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/Frameworks/"); bool IsSystem = false; EXPECT_EQ(Search.suggestPathToFileForDiagnostics( @@ -185,10 +189,22 @@ /*MainFile=*/"", &IsSystem), "AppKit/NSView.h"); EXPECT_TRUE(IsSystem); + + addFrameworkSearchDir("/System/Developer/Library/Framworks/", + /*IsSystem*/ false); + EXPECT_EQ(Search.suggestPathToFileForDiagnostics( + "/System/Developer/Library/Framworks/" + "Foo.framework/Headers/Foo.h", + /*WorkingDir=*/"", + /*MainFile=*/"", &IsSystem), + "Foo/Foo.h"); + // Expect to be true even though we passed false to IsSystem earlier since + // all frameworks should be treated as <>. + EXPECT_TRUE(IsSystem); } TEST_F(HeaderSearchTest, NestedFramework) { - addSystemFrameworkSearchDir("/Platforms/MacOSX/Frameworks"); + addFrameworkSearchDir("/Platforms/MacOSX/Frameworks"); EXPECT_EQ(Search.suggestPathToFileForDiagnostics( "/Platforms/MacOSX/Frameworks/AppKit.framework/Frameworks/" "Sub.framework/Headers/Sub.h", @@ -199,7 +215,7 @@ TEST_F(HeaderSearchTest, HeaderFrameworkLookup) { std::string HeaderPath = "/tmp/Frameworks/Foo.framework/Headers/Foo.h"; - addSystemFrameworkSearchDir("/tmp/Frameworks"); + addFrameworkSearchDir("/tmp/Frameworks"); VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBufferCopy("", HeaderPath), /*User=*/std::nullopt, /*Group=*/std::nullopt,