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