On Thu, Sep 5, 2019 at 9:57 AM Duncan P. N. Exon Smith <dexonsm...@apple.com> wrote:
> > > 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> 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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
