On Mon, Aug 26, 2019 at 11:28 AM Duncan P. N. Exon Smith via cfe-commits < cfe-commits@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@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits