Re: r369943 - FileManager: Use llvm::Expected in new getFileRef API

[David Blaikie via cfe-commits]

On Thu, Sep 5, 2019 at 9:57 AM Duncan P. N. Exon Smith 
wrote:

>
>
> On Sep 4, 2019, at 17:39, David Blaikie  wrote:
>
>
>
> 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=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?
>
>
> `getFileRef` is intended to eventually supplant `getFile` which has many
> users.  Most of them don't care about the error, they just want to know
> whether or not they have a file entry.  If it makes sense to change them at
> some point that's great, but I think having them use `getOptionalFileRef`
> makes it easy to track down (and potentially change) the ones that are
> ignoring the specific error, without requiring a ton of boilerplate at each
> call site in the meantime.  An un-posted version of the patch changed all
> the current call sites of getFileRef to handle/ignore the error explicitly
> and it looked like I was making the code worse.
>

Fair enough - thanks for the context :)


> That said, as long as we have the getOptionalFileRef API, I don't feel
> strongly about the llvm::expectedToOptional utility.  The points in favour
> are that it aligns well with llvm::errorToBool, it reduces boilerplate, and
> it seems both explicit and grep'able.  Maybe that's not compelling enough
> though.
>

I'd have objected to errorToBool on the same grounds if I'd seen the review
- and at least like Lang to take a look at llvm::Error API changes like
this to evaluate how they fit into the desire for strong error handling. I
think escape hatches from that should be implemented pretty cautiously. The
original consumeError was meant to be used very sparingly (& I see you've
provided a similar caveat on expectedToOptional (though there is none on
errorToBool) - thanks for that!

- Dave
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r369943 - FileManager: Use llvm::Expected in new getFileRef API

[Duncan P. N. Exon Smith via cfe-commits]

> On Sep 4, 2019, at 17:39, David Blaikie  wrote:
> 
> 
> 
> On Mon, Aug 26, 2019 at 11:28 AM Duncan P. N. Exon Smith via cfe-commits 
> mailto: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=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?

`getFileRef` is intended to eventually supplant `getFile` which has many users. 
 Most of them don't care about the error, they just want to know whether or not 
they have a file entry.  If it makes sense to change them at some point that's 
great, but I think having them use `getOptionalFileRef` makes it easy to track 
down (and potentially change) the ones that are ignoring the specific error, 
without requiring a ton of boilerplate at each call site in the meantime.  An 
un-posted version of the patch changed all the current call sites of getFileRef 
to handle/ignore the error explicitly and it looked like I was making the code 
worse.

That said, as long as we have the getOptionalFileRef API, I don't feel strongly 
about the llvm::expectedToOptional utility.  The points in favour are that it 
aligns well with llvm::errorToBool, it reduces boilerplate, and it seems both 
explicit and grep'able.  Maybe that's not compelling enough though.___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r369943 - FileManager: Use llvm::Expected in new getFileRef API

[David Blaikie via cfe-commits]

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=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=369942=369943=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 )
> -  : Name(Name), Entry(Entry) {}
> +  : Name(Name), Entry() {}
>
>const StringRef getName() const { return Name; }
>
> -  const FileEntry () const { return Entry; }
> +  const FileEntry () 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 () 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 
> +  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 getFileRef(StringRef Filename,
> - bool OpenFile = false,
> - bool CacheFailure = true);
> +  llvm::Expected getFileRef(StringRef Filename,
> +  bool OpenFile = false,
> +  bool CacheFailure = true);
> +
> +  /// Get a FileEntryRef if it exists, without doing anything on error.
> +  llvm::Optional getOptionalFileRef(StringRef Filename,
> +  bool OpenFile = false,
> +  bool CacheFailure =
> true) {
> +return llvm::expectedToOptional(
> +getFileRef(Filename, OpenFile, CacheFailure));
> +  }
>
>/// Returns the current file system options
>FileSystemOptions () { return FileSystemOpts; }
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=369943=369942=369943=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 >getFileEntry();
> -  return Result.getError();
> +  return llvm::errorToErrorCode(Result.takeError());
>  }
>
> -llvm::ErrorOr
> +llvm::Expected
>  FileManager::getFileRef(StringRef Filename, bool openFile, bool
> CacheFailure) {
>++NumFileLookups;
>
> @@ -199,7 +199,8 @@ FileManager::getFileRef(StringRef Filena
>SeenFileEntries.insert({Filename,
> 

r369943 - FileManager: Use llvm::Expected in new getFileRef API

[Duncan P. N. Exon Smith via cfe-commits]

Author: dexonsmith
Date: Mon Aug 26 11:29:51 2019
New Revision: 369943

URL: http://llvm.org/viewvc/llvm-project?rev=369943=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.

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=369942=369943=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 )
-  : Name(Name), Entry(Entry) {}
+  : Name(Name), Entry() {}
 
   const StringRef getName() const { return Name; }
 
-  const FileEntry () const { return Entry; }
+  const FileEntry () 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 () 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 
+  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 getFileRef(StringRef Filename,
- bool OpenFile = false,
- bool CacheFailure = true);
+  llvm::Expected getFileRef(StringRef Filename,
+  bool OpenFile = false,
+  bool CacheFailure = true);
+
+  /// Get a FileEntryRef if it exists, without doing anything on error.
+  llvm::Optional getOptionalFileRef(StringRef Filename,
+  bool OpenFile = false,
+  bool CacheFailure = true) {
+return llvm::expectedToOptional(
+getFileRef(Filename, OpenFile, CacheFailure));
+  }
 
   /// Returns the current file system options
   FileSystemOptions () { return FileSystemOpts; }

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=369943=369942=369943=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 >getFileEntry();
-  return Result.getError();
+  return llvm::errorToErrorCode(Result.takeError());
 }
 
-llvm::ErrorOr
+llvm::Expected
 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());