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 <dexonsmith at apple.com>
wrote:

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