https://github.com/benlangmuir updated https://github.com/llvm/llvm-project/pull/199759
>From 5b2e81494e298c403286f66fc86047c1ae23e917 Mon Sep 17 00:00:00 2001 From: Ben Langmuir <[email protected]> Date: Tue, 26 May 2026 13:16:25 -0700 Subject: [PATCH 1/2] Reapply "[clang] Use FileError in FileManager::getFileRef, getDirectoryRef" 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. --- clang/include/clang/Basic/FileManager.h | 37 ++++++++++-- clang/lib/Basic/FileManager.cpp | 46 ++++++++------- clang/lib/Lex/HeaderSearch.cpp | 8 +-- clang/lib/Lex/ModuleMap.cpp | 3 +- clang/lib/Lex/PPDirectives.cpp | 22 +++----- clang/unittests/Basic/FileManagerTest.cpp | 69 +++++++++++++++++++++++ 6 files changed, 135 insertions(+), 50 deletions(-) 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 >From 49365e25268df661d6ae83f1281aaa30ae17cc3d Mon Sep 17 00:00:00 2001 From: Ben Langmuir <[email protected]> Date: Wed, 27 May 2026 08:17:40 -0700 Subject: [PATCH 2/2] [fixup] Migrate doc comment for getDirectoryFromFile to header --- clang/include/clang/Basic/FileManager.h | 2 ++ clang/lib/Basic/FileManager.cpp | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h index ebdfe31d18cea..c1268befad5ae 100644 --- a/clang/include/clang/Basic/FileManager.h +++ b/clang/include/clang/Basic/FileManager.h @@ -145,6 +145,8 @@ class FileManager : public RefCountedBase<FileManager> { llvm::ErrorOr<DirectoryEntryRef> getDirectoryRefImpl(StringRef DirName, bool CacheFailure); + /// Retrieves the directory that the given \p Filename resides in. + /// \p Filename can point to either a real file or a virtual file. llvm::ErrorOr<DirectoryEntryRef> getDirectoryFromFile(StringRef Filename, bool CacheFailure); diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp index f27183ed19ac0..1648d7da19962 100644 --- a/clang/lib/Basic/FileManager.cpp +++ b/clang/lib/Basic/FileManager.cpp @@ -83,8 +83,6 @@ void FileManager::setStatCache(std::unique_ptr<FileSystemStatCache> statCache) { 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. llvm::ErrorOr<DirectoryEntryRef> FileManager::getDirectoryFromFile(StringRef Filename, bool CacheFailure) { if (Filename.empty()) _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
