dgoldman created this revision.
dgoldman added a reviewer: kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
dgoldman requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.
All callers will use the IsSystem flag to decide whether to use <> or ""
style includes, so we treat all frameworks as System since by convention
they use <> style includes.
Also update clangd's custom logic for frameworks accordingly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156704
Files:
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang/lib/Lex/HeaderSearch.cpp
clang/unittests/Lex/HeaderSearchTest.cpp
Index: clang/unittests/Lex/HeaderSearchTest.cpp
===================================================================
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ 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,
Index: clang/lib/Lex/HeaderSearch.cpp
===================================================================
--- clang/lib/Lex/HeaderSearch.cpp
+++ 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;
}
}
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ 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("<Foundation/NSObject.h>")),
AllOf(qName("PrivateClass"),
- includeHeader("\"Foundation/NSObject+Private.h\"")),
+ includeHeader("<Foundation/NSObject+Private.h>")),
AllOf(qName("Container"))));
// After adding the umbrella headers, we should use that spelling instead.
@@ -725,9 +725,9 @@
EXPECT_THAT(Symbols,
UnorderedElementsAre(
AllOf(qName("NSObject"),
- includeHeader("\"Foundation/Foundation.h\"")),
+ includeHeader("<Foundation/Foundation.h>")),
AllOf(qName("PrivateClass"),
- includeHeader("\"Foundation/Foundation_Private.h\"")),
+ includeHeader("<Foundation/Foundation_Private.h>")),
AllOf(qName("Container"))));
runSymbolCollector(Header, Main,
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -329,41 +329,31 @@
// <Foundation/NSObject_Private.h> which should be used instead of directly
// importing the header.
std::optional<std::string> getFrameworkUmbrellaSpelling(
- llvm::StringRef Framework, SrcMgr::CharacteristicKind HeadersDirKind,
- const HeaderSearch &HS, FrameworkHeaderPath &HeaderPath) {
+ 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 +376,15 @@
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);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
