[clang] 9fe632b - [clang][HeaderSearch] Treat framework headers as Angled for suggestPath
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 9 12:51:12 PDT 2023
Author: David Goldman
Date: 2023-08-09T15:51:02-04:00
New Revision: 9fe632ba18f1398aa9e6fd531a2ed98fd79eba40
URL: https://github.com/llvm/llvm-project/commit/9fe632ba18f1398aa9e6fd531a2ed98fd79eba40
DIFF: https://github.com/llvm/llvm-project/commit/9fe632ba18f1398aa9e6fd531a2ed98fd79eba40.diff
LOG: [clang][HeaderSearch] Treat framework headers as Angled for suggestPath
- Rename the IsSystem flag to be IsAngled since that's how callers
actually use the flag.
- Since frameworks by convention use <> style includes, make sure
we treat them as Angled
Also update clangd's custom logic for frameworks accordingly.
Differential Revision: https://reviews.llvm.org/D156704
Added:
Modified:
clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp
clang/include/clang/Lex/HeaderSearch.h
clang/lib/Lex/HeaderSearch.cpp
clang/lib/Sema/SemaLookup.cpp
clang/unittests/Lex/HeaderSearchTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp b/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
index 45fc15619ecab0..1bb5cf756b95b4 100644
--- a/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
+++ b/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
@@ -314,11 +314,11 @@ std::string IncludeFixerSemaSource::minimizeInclude(
if (!Entry)
return std::string(Include);
- bool IsSystem = false;
+ bool IsAngled = false;
std::string Suggestion =
- HeaderSearch.suggestPathToFileForDiagnostics(*Entry, "", &IsSystem);
+ HeaderSearch.suggestPathToFileForDiagnostics(*Entry, "", &IsAngled);
- return IsSystem ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
+ return IsAngled ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
}
/// Get the include fixer context for the queried symbol.
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 83fba21b1d931a..6005069be01160 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -286,11 +286,11 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
assert(InsertedHeader.valid());
if (InsertedHeader.Verbatim)
return InsertedHeader.File;
- bool IsSystem = false;
+ bool IsAngled = false;
std::string Suggested;
if (HeaderSearchInfo) {
Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
- InsertedHeader.File, BuildDir, IncludingFile, &IsSystem);
+ InsertedHeader.File, BuildDir, IncludingFile, &IsAngled);
} else {
// Calculate include relative to including file only.
StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
@@ -303,7 +303,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
// FIXME: should we allow (some limited number of) "../header.h"?
if (llvm::sys::path::is_absolute(Suggested))
return std::nullopt;
- if (IsSystem)
+ if (IsAngled)
Suggested = "<" + Suggested + ">";
else
Suggested = "\"" + Suggested + "\"";
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index c9a211b9c4fc2c..e843413601f5a2 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -328,42 +328,33 @@ class SymbolCollector::HeaderFileURICache {
// <Foundation/Foundation_Private.h> instead of
// <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) {
+ std::optional<std::string>
+ 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 @@ class SymbolCollector::HeaderFileURICache {
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
index 338cada5c03cbc..58b9c858010519 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -702,9 +702,9 @@ TEST_F(SymbolCollectorTest, ObjCFrameworkIncludeHeader) {
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.
@@ -722,13 +722,13 @@ TEST_F(SymbolCollectorTest, ObjCFrameworkIncludeHeader) {
"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("<Foundation/Foundation.h>")),
+ AllOf(qName("PrivateClass"),
+ includeHeader("<Foundation/Foundation_Private.h>")),
+ AllOf(qName("Container"))));
runSymbolCollector(Header, Main,
{"-iframework", FrameworksPath, "-xobjective-c++"});
diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
index 431d4bb211e0dc..1bb4080b7e2b87 100644
--- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
+++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
@@ -170,10 +170,10 @@ class Reporter {
std::string spellHeader(const Header &H) {
switch (H.kind()) {
case Header::Physical: {
- bool IsSystem = false;
+ bool IsAngled = false;
std::string Path = HS.suggestPathToFileForDiagnostics(
- H.physical(), MainFE->tryGetRealPathName(), &IsSystem);
- return IsSystem ? "<" + Path + ">" : "\"" + Path + "\"";
+ H.physical(), MainFE->tryGetRealPathName(), &IsAngled);
+ return IsAngled ? "<" + Path + ">" : "\"" + Path + "\"";
}
case Header::Standard:
return H.standard().name().str();
diff --git a/clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp b/clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp
index bb49630fe3c80b..2073f0a1d3d878 100644
--- a/clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp
+++ b/clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp
@@ -29,10 +29,10 @@ class DefaultIncludeSpeller : public IncludeSpeller {
case Header::Verbatim:
return Input.H.verbatim().str();
case Header::Physical:
- bool IsSystem = false;
+ bool IsAngled = false;
std::string FinalSpelling = Input.HS.suggestPathToFileForDiagnostics(
- Input.H.physical(), Input.Main->tryGetRealPathName(), &IsSystem);
- return IsSystem ? "<" + FinalSpelling + ">" : "\"" + FinalSpelling + "\"";
+ Input.H.physical(), Input.Main->tryGetRealPathName(), &IsAngled);
+ return IsAngled ? "<" + FinalSpelling + ">" : "\"" + FinalSpelling + "\"";
}
llvm_unreachable("Unknown clang::include_cleaner::Header::Kind enum");
}
diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h
index 1bd14283dc539d..37cdbc7519a4ef 100644
--- a/clang/include/clang/Lex/HeaderSearch.h
+++ b/clang/include/clang/Lex/HeaderSearch.h
@@ -871,11 +871,11 @@ class HeaderSearch {
/// MainFile location, if none of the include search directories were prefix
/// of File.
///
- /// \param IsSystem If non-null, filled in to indicate whether the suggested
- /// path is relative to a system header directory.
+ /// \param IsAngled If non-null, filled in to indicate whether the suggested
+ /// path should be referenced as <Header.h> instead of "Header.h".
std::string suggestPathToFileForDiagnostics(const FileEntry *File,
llvm::StringRef MainFile,
- bool *IsSystem = nullptr) const;
+ bool *IsAngled = nullptr) const;
/// Suggest a path by which the specified file could be found, for use in
/// diagnostics to suggest a #include. Returned path will only contain forward
@@ -889,7 +889,7 @@ class HeaderSearch {
std::string suggestPathToFileForDiagnostics(llvm::StringRef File,
llvm::StringRef WorkingDir,
llvm::StringRef MainFile,
- bool *IsSystem = nullptr) const;
+ bool *IsAngled = nullptr) const;
void PrintStats();
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index a5e8b028b25ecf..f42d51d65dcdf3 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1928,17 +1928,17 @@ void HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir) {
}
std::string HeaderSearch::suggestPathToFileForDiagnostics(
- const FileEntry *File, llvm::StringRef MainFile, bool *IsSystem) const {
+ const FileEntry *File, llvm::StringRef MainFile, bool *IsAngled) const {
// FIXME: We assume that the path name currently cached in the FileEntry is
// the most appropriate one for this analysis (and that it's spelled the
// same way as the corresponding header search path).
return suggestPathToFileForDiagnostics(File->getName(), /*WorkingDir=*/"",
- MainFile, IsSystem);
+ MainFile, IsAngled);
}
std::string HeaderSearch::suggestPathToFileForDiagnostics(
llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
- bool *IsSystem) const {
+ bool *IsAngled) const {
using namespace llvm::sys;
llvm::SmallString<32> FilePath = File;
@@ -1996,15 +1996,16 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
if (DL.isNormalDir()) {
StringRef Dir = DL.getDirRef()->getName();
if (CheckDir(Dir)) {
- if (IsSystem)
- *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic());
+ if (IsAngled)
+ *IsAngled = BestPrefixLength && isSystem(DL.getDirCharacteristic());
BestPrefixIsFramework = false;
}
} else if (DL.isFramework()) {
StringRef Dir = DL.getFrameworkDirRef()->getName();
if (CheckDir(Dir)) {
- if (IsSystem)
- *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic());
+ // Framework includes by convention use <>.
+ if (IsAngled)
+ *IsAngled = BestPrefixLength;
BestPrefixIsFramework = true;
}
}
@@ -2013,8 +2014,8 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
// Try to shorten include path using TUs directory, if we couldn't find any
// suitable prefix in include search paths.
if (!BestPrefixLength && CheckDir(path::parent_path(MainFile))) {
- if (IsSystem)
- *IsSystem = false;
+ if (IsAngled)
+ *IsAngled = false;
BestPrefixIsFramework = false;
}
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 4cf45375e4ecc9..05a6618cd0d1e6 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -5701,10 +5701,10 @@ void Sema::diagnoseMissingImport(SourceLocation Loc, const NamedDecl *Decl,
/// suggesting the addition of a #include of the specified file.
static std::string getHeaderNameForHeader(Preprocessor &PP, const FileEntry *E,
llvm::StringRef IncludingFile) {
- bool IsSystem = false;
+ bool IsAngled = false;
auto Path = PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(
- E, IncludingFile, &IsSystem);
- return (IsSystem ? '<' : '"') + Path + (IsSystem ? '>' : '"');
+ E, IncludingFile, &IsAngled);
+ return (IsAngled ? '<' : '"') + Path + (IsAngled ? '>' : '"');
}
void Sema::diagnoseMissingImport(SourceLocation UseLoc, const NamedDecl *Decl,
diff --git a/clang/unittests/Lex/HeaderSearchTest.cpp b/clang/unittests/Lex/HeaderSearchTest.cpp
index b0f4221ad335b0..cc30b0a4304ff3 100644
--- a/clang/unittests/Lex/HeaderSearchTest.cpp
+++ b/clang/unittests/Lex/HeaderSearchTest.cpp
@@ -47,14 +47,18 @@ class HeaderSearchTest : public ::testing::Test {
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,20 +179,32 @@ TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
}
TEST_F(HeaderSearchTest, SdkFramework) {
- addSystemFrameworkSearchDir(
+ addFrameworkSearchDir(
"/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/Frameworks/");
- bool IsSystem = false;
+ bool IsAngled = false;
EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
"/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"
"Frameworks/AppKit.framework/Headers/NSView.h",
/*WorkingDir=*/"",
- /*MainFile=*/"", &IsSystem),
+ /*MainFile=*/"", &IsAngled),
"AppKit/NSView.h");
- EXPECT_TRUE(IsSystem);
+ EXPECT_TRUE(IsAngled);
+
+ addFrameworkSearchDir("/System/Developer/Library/Framworks/",
+ /*IsSystem*/ false);
+ EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+ "/System/Developer/Library/Framworks/"
+ "Foo.framework/Headers/Foo.h",
+ /*WorkingDir=*/"",
+ /*MainFile=*/"", &IsAngled),
+ "Foo/Foo.h");
+ // Expect to be true even though we passed false to IsSystem earlier since
+ // all frameworks should be treated as <>.
+ EXPECT_TRUE(IsAngled);
}
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, NestedFramework) {
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,
More information about the cfe-commits
mailing list