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 <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Aug 26, 2019 at 11:28 AM Duncan P. N. Exon Smith via cfe-commits <cfe-commits at lists.llvm.org <mailto: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 <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?

`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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190905/a5422de0/attachment.html>