> On Sep 4, 2019, at 17:39, David Blaikie <dblai...@gmail.com> wrote:
>
>
>
> On Mon, Aug 26, 2019 at 11:28 AM Duncan P. N. Exon Smith via cfe-commits
> <cfe-commits@lists.llvm.org <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&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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
