r369943 - FileManager: Use llvm::Expected in new getFileRef API
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 4 17:39:39 PDT 2019
On Mon, Aug 26, 2019 at 11:28 AM Duncan P. N. Exon Smith via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Author: dexonsmith
> Date: Mon Aug 26 11:29:51 2019
> New Revision: 369943
>
> URL: http://llvm.org/viewvc/llvm-project?rev=369943&view=rev
> Log:
> FileManager: Use llvm::Expected in new getFileRef API
>
> `FileManager::getFileRef` is a modern API which we expect to convert to
> over time. We should modernize the error handling as well, using
> `llvm::Expected` instead of `llvm::ErrorOr`, to help clients that care
> about errors to ensure nothing is missed.
>
> However, not all clients care. I've also added another path for those
> that don't:
>
> - `FileEntryRef` is now copy- and move-assignable (using a pointer
> instead of a reference).
> - `FileManager::getOptionalFileRef` returns an `llvm::Optional` instead
> of `llvm::Expected`.
> - Added an `llvm::expectedToOptional` utility in case this is useful
> elsewhere.
>
I'd hesitate to add new general constructs that swallow errors like this -
keeping them manually written might help avoid their use becoming too
common.
On that note/direction - are there enough callers of getFileRef that don't
care about errors that it's really impractical for them to each explicitly
swallow the errors?
>
> https://reviews.llvm.org/D66705
>
> Modified:
> cfe/trunk/include/clang/Basic/FileManager.h
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/lib/Frontend/CompilerInstance.cpp
> cfe/trunk/lib/Lex/HeaderMap.cpp
> cfe/trunk/lib/Lex/HeaderSearch.cpp
>
> Modified: cfe/trunk/include/clang/Basic/FileManager.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=369943&r1=369942&r2=369943&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/FileManager.h (original)
> +++ cfe/trunk/include/clang/Basic/FileManager.h Mon Aug 26 11:29:51 2019
> @@ -110,26 +110,27 @@ public:
> /// accessed by the FileManager's client.
> class FileEntryRef {
> public:
> + FileEntryRef() = delete;
> FileEntryRef(StringRef Name, const FileEntry &Entry)
> - : Name(Name), Entry(Entry) {}
> + : Name(Name), Entry(&Entry) {}
>
> const StringRef getName() const { return Name; }
>
> - const FileEntry &getFileEntry() const { return Entry; }
> + const FileEntry &getFileEntry() const { return *Entry; }
>
> - off_t getSize() const { return Entry.getSize(); }
> + off_t getSize() const { return Entry->getSize(); }
>
> - unsigned getUID() const { return Entry.getUID(); }
> + unsigned getUID() const { return Entry->getUID(); }
>
> const llvm::sys::fs::UniqueID &getUniqueID() const {
> - return Entry.getUniqueID();
> + return Entry->getUniqueID();
> }
>
> - time_t getModificationTime() const { return
> Entry.getModificationTime(); }
> + time_t getModificationTime() const { return
> Entry->getModificationTime(); }
>
> private:
> StringRef Name;
> - const FileEntry &Entry;
> + const FileEntry *Entry;
> };
>
> /// Implements support for file system lookup, file system caching,
> @@ -284,9 +285,17 @@ public:
> ///
> /// \param CacheFailure If true and the file does not exist, we'll cache
> /// the failure to find this file.
> - llvm::ErrorOr<FileEntryRef> getFileRef(StringRef Filename,
> - bool OpenFile = false,
> - bool CacheFailure = true);
> + llvm::Expected<FileEntryRef> getFileRef(StringRef Filename,
> + bool OpenFile = false,
> + bool CacheFailure = true);
> +
> + /// Get a FileEntryRef if it exists, without doing anything on error.
> + llvm::Optional<FileEntryRef> getOptionalFileRef(StringRef Filename,
> + bool OpenFile = false,
> + bool CacheFailure =
> true) {
> + return llvm::expectedToOptional(
> + getFileRef(Filename, OpenFile, CacheFailure));
> + }
>
> /// Returns the current file system options
> FileSystemOptions &getFileSystemOpts() { return FileSystemOpts; }
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=369943&r1=369942&r2=369943&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Mon Aug 26 11:29:51 2019
> @@ -187,10 +187,10 @@ FileManager::getFile(StringRef Filename,
> auto Result = getFileRef(Filename, openFile, CacheFailure);
> if (Result)
> return &Result->getFileEntry();
> - return Result.getError();
> + return llvm::errorToErrorCode(Result.takeError());
> }
>
> -llvm::ErrorOr<FileEntryRef>
> +llvm::Expected<FileEntryRef>
> FileManager::getFileRef(StringRef Filename, bool openFile, bool
> CacheFailure) {
> ++NumFileLookups;
>
> @@ -199,7 +199,8 @@ FileManager::getFileRef(StringRef Filena
> SeenFileEntries.insert({Filename,
> std::errc::no_such_file_or_directory});
> if (!SeenFileInsertResult.second) {
> if (!SeenFileInsertResult.first->second)
> - return SeenFileInsertResult.first->second.getError();
> + return llvm::errorCodeToError(
> + SeenFileInsertResult.first->second.getError());
> // Construct and return and FileEntryRef, unless it's a redirect to
> another
> // filename.
> SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
> @@ -230,7 +231,7 @@ FileManager::getFileRef(StringRef Filena
> else
> SeenFileEntries.erase(Filename);
>
> - return DirInfoOrErr.getError();
> + return llvm::errorCodeToError(DirInfoOrErr.getError());
> }
> const DirectoryEntry *DirInfo = *DirInfoOrErr;
>
> @@ -249,7 +250,7 @@ FileManager::getFileRef(StringRef Filena
> else
> SeenFileEntries.erase(Filename);
>
> - return statError;
> + return llvm::errorCodeToError(statError);
> }
>
> assert((openFile || !F) && "undesired open file");
>
> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=369943&r1=369942&r2=369943&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Mon Aug 26 11:29:51 2019
> @@ -833,6 +833,8 @@ bool CompilerInstance::InitializeSourceM
> if (InputFile != "-") {
> auto FileOrErr = FileMgr.getFileRef(InputFile, /*OpenFile=*/true);
> if (!FileOrErr) {
> + // FIXME: include the error in the diagnostic.
> + consumeError(FileOrErr.takeError());
> Diags.Report(diag::err_fe_error_reading) << InputFile;
> return false;
> }
>
> Modified: cfe/trunk/lib/Lex/HeaderMap.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/HeaderMap.cpp?rev=369943&r1=369942&r2=369943&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Lex/HeaderMap.cpp (original)
> +++ cfe/trunk/lib/Lex/HeaderMap.cpp Mon Aug 26 11:29:51 2019
> @@ -204,9 +204,7 @@ Optional<FileEntryRef> HeaderMap::Lookup
> if (Dest.empty())
> return None;
>
> - if (auto File = FM.getFileRef(Dest))
> - return *File;
> - return None;
> + return FM.getOptionalFileRef(Dest);
> }
>
> StringRef HeaderMapImpl::lookupFilename(StringRef Filename,
>
> Modified: cfe/trunk/lib/Lex/HeaderSearch.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/HeaderSearch.cpp?rev=369943&r1=369942&r2=369943&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Lex/HeaderSearch.cpp (original)
> +++ cfe/trunk/lib/Lex/HeaderSearch.cpp Mon Aug 26 11:29:51 2019
> @@ -314,7 +314,7 @@ Optional<FileEntryRef> HeaderSearch::get
> if (!File) {
> // For rare, surprising errors (e.g. "out of file handles"), diag the
> EC
> // message.
> - std::error_code EC = File.getError();
> + std::error_code EC = llvm::errorToErrorCode(File.takeError());
> if (EC != llvm::errc::no_such_file_or_directory &&
> EC != llvm::errc::invalid_argument &&
> EC != llvm::errc::is_a_directory && EC !=
> llvm::errc::not_a_directory) {
> @@ -401,7 +401,7 @@ Optional<FileEntryRef> DirectoryLookup::
> FixupSearchPath();
> return *Result;
> }
> - } else if (auto Res = HS.getFileMgr().getFileRef(Dest)) {
> + } else if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest)) {
> FixupSearchPath();
> return *Res;
> }
> @@ -553,9 +553,8 @@ Optional<FileEntryRef> DirectoryLookup::
>
> FrameworkName.append(Filename.begin()+SlashPos+1, Filename.end());
>
> - llvm::ErrorOr<FileEntryRef> File =
> - FileMgr.getFileRef(FrameworkName, /*OpenFile=*/!SuggestedModule);
> -
> + auto File =
> + FileMgr.getOptionalFileRef(FrameworkName,
> /*OpenFile=*/!SuggestedModule);
> if (!File) {
> // Check
> "/System/Library/Frameworks/Cocoa.framework/PrivateHeaders/file.h"
> const char *Private = "Private";
> @@ -565,7 +564,8 @@ Optional<FileEntryRef> DirectoryLookup::
> SearchPath->insert(SearchPath->begin()+OrigSize, Private,
> Private+strlen(Private));
>
> - File = FileMgr.getFileRef(FrameworkName,
> /*OpenFile=*/!SuggestedModule);
> + File = FileMgr.getOptionalFileRef(FrameworkName,
> + /*OpenFile=*/!SuggestedModule);
> }
>
> // If we found the header and are allowed to suggest a module, do so
> now.
> @@ -1076,9 +1076,7 @@ Optional<FileEntryRef> HeaderSearch::Loo
> }
>
> HeadersFilename.append(Filename.begin()+SlashPos+1, Filename.end());
> - llvm::ErrorOr<FileEntryRef> File =
> - FileMgr.getFileRef(HeadersFilename, /*OpenFile=*/true);
> -
> + auto File = FileMgr.getOptionalFileRef(HeadersFilename,
> /*OpenFile=*/true);
> if (!File) {
> // Check
> ".../Frameworks/HIToolbox.framework/PrivateHeaders/HIToolbox.h"
> HeadersFilename = FrameworkName;
> @@ -1090,7 +1088,7 @@ Optional<FileEntryRef> HeaderSearch::Loo
> }
>
> HeadersFilename.append(Filename.begin()+SlashPos+1, Filename.end());
> - File = FileMgr.getFileRef(HeadersFilename, /*OpenFile=*/true);
> + File = FileMgr.getOptionalFileRef(HeadersFilename, /*OpenFile=*/true);
>
> if (!File)
> return None;
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190904/ae77b909/attachment-0001.html>
More information about the cfe-commits
mailing list