Reapply "[clang] Use FileError in FileManager::getFileRef, getDirectoryRef"#199759
Open
benlangmuir wants to merge 1 commit into
Open
Reapply "[clang] Use FileError in FileManager::getFileRef, getDirectoryRef"#199759benlangmuir wants to merge 1 commit into
benlangmuir wants to merge 1 commit into
Conversation
…ryRef" Most callers are unchanged, since they either ignore the specific error or have their own formatting of the error that includes both the path and the errorToErrorCode-unwrapped value. However, for clients that just forward the error it's helpful to ensure we do not lose track of the filename that the error is associated with, so use FileError. To reduce the overhead of using FileError, keep it only in the public getFileRef and getDirectoryRef themselves, and use ErrorOr in the implementation. For callers of getOptional* this should avoid allocations for errors entirely, and for callers of getFileRef and getDirectoryRef it makes the error allocation inlinable, which may (in theory, not tested) let us optimize it away if the Error is immediately unwrapped back to an error code, for example. Incidentally clean up some callers to use getOptionalFileRef where they throw away the error immediately.
llvmorg-github-actions
Bot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Ben Langmuir (benlangmuir) ChangesMost callers are unchanged, since they either ignore the specific error or have their own formatting of the error that includes both the path and the errorToErrorCode-unwrapped value. However, for clients that just forward the error it's helpful to ensure we do not lose track of the filename that the error is associated with, so use FileError. To reduce the overhead of using FileError, keep it only in the public getFileRef and getDirectoryRef themselves, and use ErrorOr in the implementation. For callers of getOptional* this should avoid allocations for errors entirely, and for callers of getFileRef and getDirectoryRef it makes the error allocation inlinable, which may (in theory, not tested) let us optimize it away if the Error is immediately unwrapped back to an error code, for example. Incidentally clean up some callers to use getOptionalFileRef where they throw away the error immediately. Full diff: https://github.com/llvm/llvm-project/pull/199759.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index e440a57e3a866..ebdfe31d18cea 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -31,6 +31,7 @@
#include <ctime>
#include <map>
#include <memory>
+#include <optional>
#include <string>
namespace llvm {
@@ -134,6 +135,19 @@ class FileManager : public RefCountedBase<FileManager> {
/// Fills the RealPathName in file entry.
void fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+ /// Implementation for getFileRef and getOptionalFileRef. Uses \c ErrorOr for
+ /// efficiency when an error will be ignored.
+ llvm::ErrorOr<FileEntryRef> getFileRefImpl(StringRef Filename, bool OpenFile,
+ bool CacheFailure, bool IsText);
+
+ /// Implementation for getDirectoryRef and getOptionalDirectoryRef. Uses
+ /// \c ErrorOr for efficiency when an error will be ignored.
+ llvm::ErrorOr<DirectoryEntryRef> getDirectoryRefImpl(StringRef DirName,
+ bool CacheFailure);
+
+ llvm::ErrorOr<DirectoryEntryRef> getDirectoryFromFile(StringRef Filename,
+ bool CacheFailure);
+
public:
/// Construct a file manager, optionally with a custom VFS.
///
@@ -169,12 +183,19 @@ class FileManager : public RefCountedBase<FileManager> {
/// \param CacheFailure If true and the file does not exist, we'll cache
/// the failure to find this file.
llvm::Expected<DirectoryEntryRef> getDirectoryRef(StringRef DirName,
- bool CacheFailure = true);
+ bool CacheFailure = true) {
+ auto Ref = getDirectoryRefImpl(DirName, CacheFailure);
+ if (Ref)
+ return *Ref;
+ return llvm::createFileError(DirName, Ref.getError());
+ }
/// Get a \c DirectoryEntryRef if it exists, without doing anything on error.
OptionalDirectoryEntryRef getOptionalDirectoryRef(StringRef DirName,
bool CacheFailure = true) {
- return llvm::expectedToOptional(getDirectoryRef(DirName, CacheFailure));
+ if (auto Ref = getDirectoryRefImpl(DirName, CacheFailure))
+ return *Ref;
+ return std::nullopt;
}
/// Lookup, cache, and verify the specified file (real or virtual). Return the
@@ -194,7 +215,12 @@ class FileManager : public RefCountedBase<FileManager> {
llvm::Expected<FileEntryRef> getFileRef(StringRef Filename,
bool OpenFile = false,
bool CacheFailure = true,
- bool IsText = true);
+ bool IsText = true) {
+ auto Ref = getFileRefImpl(Filename, OpenFile, CacheFailure, IsText);
+ if (Ref)
+ return *Ref;
+ return llvm::createFileError(Filename, Ref.getError());
+ }
/// Get the FileEntryRef for stdin, returning an error if stdin cannot be
/// read.
@@ -209,8 +235,9 @@ class FileManager : public RefCountedBase<FileManager> {
bool OpenFile = false,
bool CacheFailure = true,
bool IsText = true) {
- return llvm::expectedToOptional(
- getFileRef(Filename, OpenFile, CacheFailure, IsText));
+ if (auto Ref = getFileRefImpl(Filename, OpenFile, CacheFailure, IsText))
+ return *Ref;
+ return std::nullopt;
}
/// Returns the current file system options
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index a126d14087963..f27183ed19ac0 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -21,6 +21,7 @@
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/IOSandbox.h"
#include "llvm/Support/MemoryBuffer.h"
@@ -84,22 +85,20 @@ void FileManager::clearStatCache() { StatCache.reset(); }
/// Retrieve the directory that the given file name resides in.
/// Filename can point to either a real file or a virtual file.
-static llvm::Expected<DirectoryEntryRef>
-getDirectoryFromFile(FileManager &FileMgr, StringRef Filename,
- bool CacheFailure) {
+llvm::ErrorOr<DirectoryEntryRef>
+FileManager::getDirectoryFromFile(StringRef Filename, bool CacheFailure) {
if (Filename.empty())
- return llvm::errorCodeToError(
- make_error_code(std::errc::no_such_file_or_directory));
+ return make_error_code(std::errc::no_such_file_or_directory);
if (llvm::sys::path::is_separator(Filename[Filename.size() - 1]))
- return llvm::errorCodeToError(make_error_code(std::errc::is_a_directory));
+ return make_error_code(std::errc::is_a_directory);
StringRef DirName = llvm::sys::path::parent_path(Filename);
// Use the current directory if file has no path component.
if (DirName.empty())
DirName = ".";
- return FileMgr.getDirectoryRef(DirName, CacheFailure);
+ return getDirectoryRefImpl(DirName, CacheFailure);
}
DirectoryEntry *&FileManager::getRealDirEntry(const llvm::vfs::Status &Status) {
@@ -162,8 +161,8 @@ void FileManager::addAncestorsAsVirtualDirs(StringRef Path) {
addAncestorsAsVirtualDirs(OriginalDirName);
}
-llvm::Expected<DirectoryEntryRef>
-FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
+llvm::ErrorOr<DirectoryEntryRef>
+FileManager::getDirectoryRefImpl(StringRef DirName, bool CacheFailure) {
std::optional<std::string> DirNameStr;
normalizeCacheKey(DirName, DirNameStr);
@@ -176,7 +175,7 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
if (!SeenDirInsertResult.second) {
if (SeenDirInsertResult.first->second)
return DirectoryEntryRef(*SeenDirInsertResult.first);
- return llvm::errorCodeToError(SeenDirInsertResult.first->second.getError());
+ return SeenDirInsertResult.first->second.getError();
}
// We've not seen this before. Fill it in.
@@ -198,7 +197,7 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
NamedDirEnt.second = statError;
else
SeenDirEntries.erase(DirName);
- return llvm::errorCodeToError(statError);
+ return statError;
}
// It exists.
@@ -208,10 +207,10 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
return DirectoryEntryRef(NamedDirEnt);
}
-llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
- bool openFile,
- bool CacheFailure,
- bool IsText) {
+llvm::ErrorOr<FileEntryRef> FileManager::getFileRefImpl(StringRef Filename,
+ bool openFile,
+ bool CacheFailure,
+ bool IsText) {
++NumFileLookups;
// See if there is already an entry in the map.
@@ -219,8 +218,7 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
SeenFileEntries.insert({Filename, std::errc::no_such_file_or_directory});
if (!SeenFileInsertResult.second) {
if (!SeenFileInsertResult.first->second)
- return llvm::errorCodeToError(
- SeenFileInsertResult.first->second.getError());
+ return SeenFileInsertResult.first->second.getError();
return FileEntryRef(*SeenFileInsertResult.first);
}
@@ -238,15 +236,15 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
// subdirectory. This will let us avoid having to waste time on known-to-fail
// searches when we go to find sys/bar.h, because all the search directories
// without a 'sys' subdir will get a cached failure result.
- auto DirInfoOrErr = getDirectoryFromFile(*this, Filename, CacheFailure);
+ auto DirInfoOrErr = getDirectoryFromFile(Filename, CacheFailure);
if (!DirInfoOrErr) { // Directory doesn't exist, file can't exist.
- std::error_code Err = errorToErrorCode(DirInfoOrErr.takeError());
+ std::error_code Err = DirInfoOrErr.getError();
if (CacheFailure)
NamedFileEnt->second = Err;
else
SeenFileEntries.erase(Filename);
- return llvm::errorCodeToError(Err);
+ return Err;
}
DirectoryEntryRef DirInfo = *DirInfoOrErr;
@@ -265,7 +263,7 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
else
SeenFileEntries.erase(Filename);
- return llvm::errorCodeToError(statError);
+ return statError;
}
assert((openFile || !F) && "undesired open file");
@@ -367,7 +365,7 @@ llvm::Expected<FileEntryRef> FileManager::getSTDIN() {
}();
if (!ContentOrError)
- return llvm::errorCodeToError(ContentOrError.getError());
+ return llvm::createFileError("-", ContentOrError.getError());
auto Content = std::move(*ContentOrError);
STDIN = getVirtualFileRef(Content->getBufferIdentifier(),
@@ -410,8 +408,8 @@ FileEntryRef FileManager::getVirtualFileRef(StringRef Filename, off_t Size,
// from a source location preprocessor directive with an empty filename as
// an example, so we need to pretend it has a name to ensure a valid directory
// entry can be returned.
- auto DirInfo = expectedToOptional(getDirectoryFromFile(
- *this, Filename.empty() ? "." : Filename, /*CacheFailure=*/true));
+ auto DirInfo = getDirectoryFromFile(Filename.empty() ? "." : Filename,
+ /*CacheFailure=*/true);
assert(DirInfo &&
"The directory of a virtual file should already be in the cache.");
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 5cc2c04a68077..782b789f9260b 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -907,15 +907,13 @@ void HeaderSearch::diagnoseHeaderShadowing(
const auto &IncluderAndDir = Includers[i];
SmallString<1024> TmpDir = IncluderAndDir.second.getName();
llvm::sys::path::append(TmpDir, Filename);
- if (auto File = getFileMgr().getFileRef(TmpDir, false, false)) {
+ if (auto File = getFileMgr().getOptionalFileRef(TmpDir, false, false)) {
if (&File->getFileEntry() == *FE)
continue;
Diags.Report(IncludeLoc, diag::warn_header_shadowing)
<< Filename << (*FE).getDir().getName()
<< IncluderAndDir.second.getName();
return;
- } else {
- llvm::errorToErrorCode(File.takeError());
}
}
}
@@ -934,14 +932,12 @@ void HeaderSearch::diagnoseHeaderShadowing(
continue;
SmallString<1024> TmpPath = It->getName();
llvm::sys::path::append(TmpPath, Filename);
- if (auto File = getFileMgr().getFileRef(TmpPath, false, false)) {
+ if (auto File = getFileMgr().getOptionalFileRef(TmpPath, false, false)) {
if (&File->getFileEntry() == *FE)
continue;
Diags.Report(IncludeLoc, diag::warn_header_shadowing)
<< Filename << (*FE).getDir().getName() << It->getName();
return;
- } else {
- llvm::errorToErrorCode(File.takeError());
}
}
}
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 436b8e5620765..6c07386f89010 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -180,8 +180,7 @@ OptionalFileEntryRef ModuleMap::findHeader(
SmallString<128> FullPathName(Directory->getName());
auto GetFile = [&](StringRef Filename) -> OptionalFileEntryRef {
- auto File =
- expectedToOptional(SourceMgr.getFileManager().getFileRef(Filename));
+ auto File = SourceMgr.getFileManager().getOptionalFileRef(Filename);
if (!File || (Header.Size && File->getSize() != *Header.Size) ||
(Header.ModTime && File->getModificationTime() != *Header.ModTime))
return std::nullopt;
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 12d3c765b15bc..eb21a510dcf83 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -1195,9 +1195,8 @@ OptionalFileEntryRef Preprocessor::LookupEmbedFile(StringRef Filename,
FileManager &FM = this->getFileManager();
if (llvm::sys::path::is_absolute(Filename)) {
// lookup path or immediately fail
- llvm::Expected<FileEntryRef> ShouldBeEntry = FM.getFileRef(
- Filename, OpenFile, /*CacheFailure=*/true, /*IsText=*/false);
- return llvm::expectedToOptional(std::move(ShouldBeEntry));
+ return FM.getOptionalFileRef(Filename, OpenFile, /*CacheFailure=*/true,
+ /*IsText=*/false);
}
auto SeparateComponents = [](SmallVectorImpl<char> &LookupPath,
@@ -1224,27 +1223,25 @@ OptionalFileEntryRef Preprocessor::LookupEmbedFile(StringRef Filename,
TmpDir = LookupFromFile->getDir().getName();
llvm::sys::path::append(TmpDir, Filename);
if (!TmpDir.empty()) {
- llvm::Expected<FileEntryRef> ShouldBeEntry = FM.getFileRef(
+ OptionalFileEntryRef ShouldBeEntry = FM.getOptionalFileRef(
TmpDir, OpenFile, /*CacheFailure=*/true, /*IsText=*/false);
if (ShouldBeEntry)
- return llvm::expectedToOptional(std::move(ShouldBeEntry));
- llvm::consumeError(ShouldBeEntry.takeError());
+ return ShouldBeEntry;
}
}
// Otherwise, do working directory lookup.
LookupPath.clear();
- auto MaybeWorkingDirEntry = FM.getDirectoryRef(".");
+ auto MaybeWorkingDirEntry = FM.getOptionalDirectoryRef(".");
if (MaybeWorkingDirEntry) {
DirectoryEntryRef WorkingDirEntry = *MaybeWorkingDirEntry;
StringRef WorkingDir = WorkingDirEntry.getName();
if (!WorkingDir.empty()) {
SeparateComponents(LookupPath, WorkingDir, Filename, false);
- llvm::Expected<FileEntryRef> ShouldBeEntry = FM.getFileRef(
+ OptionalFileEntryRef ShouldBeEntry = FM.getOptionalFileRef(
LookupPath, OpenFile, /*CacheFailure=*/true, /*IsText=*/false);
if (ShouldBeEntry)
- return llvm::expectedToOptional(std::move(ShouldBeEntry));
- llvm::consumeError(ShouldBeEntry.takeError());
+ return ShouldBeEntry;
}
}
}
@@ -1252,11 +1249,10 @@ OptionalFileEntryRef Preprocessor::LookupEmbedFile(StringRef Filename,
for (const auto &Entry : PPOpts.EmbedEntries) {
LookupPath.clear();
SeparateComponents(LookupPath, Entry, Filename, false);
- llvm::Expected<FileEntryRef> ShouldBeEntry = FM.getFileRef(
+ OptionalFileEntryRef ShouldBeEntry = FM.getOptionalFileRef(
LookupPath, OpenFile, /*CacheFailure=*/true, /*IsText=*/false);
if (ShouldBeEntry)
- return llvm::expectedToOptional(std::move(ShouldBeEntry));
- llvm::consumeError(ShouldBeEntry.takeError());
+ return ShouldBeEntry;
}
return std::nullopt;
}
diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp
index e54d3f8f9e416..c3f306d023fb0 100644
--- a/clang/unittests/Basic/FileManagerTest.cpp
+++ b/clang/unittests/Basic/FileManagerTest.cpp
@@ -228,6 +228,75 @@ TEST_F(FileManagerTest, getFileReturnsErrorForNonexistentFile) {
std::make_error_code(std::errc::not_a_directory));
}
+TEST_F(FileManagerTest, getFileRefErrorIncludesFilename) {
+ auto FS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ auto EmptyBuffer = llvm::MemoryBuffer::getMemBuffer("");
+ ASSERT_TRUE(
+ FS->addFileNoOwn("/MyDirectory/file", 0, EmptyBuffer->getMemBufferRef()));
+ FileSystemOptions Opts;
+ FileManager Mgr(Opts, FS);
+
+ // Build the expected message for a given filename and error code, since the
+ // system-provided message text (and capitalization) for std::errc values
+ // varies by platform.
+ auto ExpectedMsg = [](StringRef Name, std::errc EC) {
+ return ("'" + Name + "': " + std::make_error_code(EC).message()).str();
+ };
+
+ // Nonexistent file.
+ auto Missing = Mgr.getFileRef("/xyz.txt");
+ ASSERT_FALSE(Missing);
+ EXPECT_EQ(ExpectedMsg("/xyz.txt", std::errc::no_such_file_or_directory),
+ llvm::toString(Missing.takeError()));
+
+ // Cached failure
+ auto MissingAgain = Mgr.getFileRef("/xyz.txt");
+ ASSERT_FALSE(MissingAgain);
+ EXPECT_EQ(std::make_error_code(std::errc::no_such_file_or_directory),
+ llvm::errorToErrorCode(MissingAgain.takeError()));
+
+ // Reading a directory as a file.
+ auto DirAsFile = Mgr.getFileRef("/MyDirectory");
+ ASSERT_FALSE(DirAsFile);
+ EXPECT_EQ(ExpectedMsg("/MyDirectory", std::errc::is_a_directory),
+ llvm::toString(DirAsFile.takeError()));
+
+ auto Trailing = Mgr.getFileRef("/some/dir/");
+ ASSERT_FALSE(Trailing);
+ EXPECT_EQ(ExpectedMsg("/some/dir/", std::errc::is_a_directory),
+ llvm::toString(Trailing.takeError()));
+}
+
+TEST_F(FileManagerTest, getDirectoryRefErrorIncludesFilename) {
+ auto FS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ auto EmptyBuffer = llvm::MemoryBuffer::getMemBuffer("");
+ ASSERT_TRUE(FS->addFileNoOwn("/foo.cpp", 0, EmptyBuffer->getMemBufferRef()));
+ FileSystemOptions Opts;
+ FileManager Mgr(Opts, FS);
+
+ auto ExpectedMsg = [](StringRef Name, std::errc EC) {
+ return ("'" + Name + "': " + std::make_error_code(EC).message()).str();
+ };
+
+ // Nonexistent directory.
+ auto Missing = Mgr.getDirectoryRef("/no_such_dir");
+ ASSERT_FALSE(Missing);
+ EXPECT_EQ(ExpectedMsg("/no_such_dir", std::errc::no_such_file_or_directory),
+ llvm::toString(Missing.takeError()));
+
+ // Cached failure
+ auto MissingAgain = Mgr.getDirectoryRef("/no_such_dir");
+ ASSERT_FALSE(MissingAgain);
+ EXPECT_EQ(std::make_error_code(std::errc::no_such_file_or_directory),
+ llvm::errorToErrorCode(MissingAgain.takeError()));
+
+ // Reading a file as a directory.
+ auto FileAsDir = Mgr.getDirectoryRef("/foo.cpp");
+ ASSERT_FALSE(FileAsDir);
+ EXPECT_EQ(ExpectedMsg("/foo.cpp", std::errc::not_a_directory),
+ llvm::toString(FileAsDir.takeError()));
+}
+
// The following tests apply to Unix-like system only.
#ifndef _WIN32
|
Contributor
Author
|
The updated performance is https://llvm-compile-time-tracker.com/compare.php?from=3ce7b405579d64f8e26e14c6cc02df92117af330&to=5b2e81494e298c403286f66fc86047c1ae23e917&stat=instructions:u which shows a much smaller geomean regression compared to the original. |
cachemeifyoucan
approved these changes
May 26, 2026
| llvm::ErrorOr<DirectoryEntryRef> getDirectoryRefImpl(StringRef DirName, | ||
| bool CacheFailure); | ||
|
|
||
| llvm::ErrorOr<DirectoryEntryRef> getDirectoryFromFile(StringRef Filename, |
MaskRay
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Most callers are unchanged, since they either ignore the specific error or have their own formatting of the error that includes both the path and the errorToErrorCode-unwrapped value. However, for clients that just forward the error it's helpful to ensure we do not lose track of the filename that the error is associated with, so use FileError.
To reduce the overhead of using FileError, keep it only in the public getFileRef and getDirectoryRef themselves, and use ErrorOr in the implementation. For callers of getOptional* this should avoid allocations for errors entirely, and for callers of getFileRef and getDirectoryRef it makes the error allocation inlinable, which may (in theory, not tested) let us optimize it away if the Error is immediately unwrapped back to an error code, for example.
Incidentally clean up some callers to use getOptionalFileRef where they throw away the error immediately.