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