<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 5, 2019 at 9:57 AM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><br><div><br><blockquote type="cite"><div>On Sep 4, 2019, at 17:39, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="gmail-m_-2189833436213773647Apple-interchange-newline"><div><br class="gmail-m_-2189833436213773647Apple-interchange-newline"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration:none"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration:none"><div dir="ltr" class="gmail_attr">On Mon, Aug 26, 2019 at 11:28 AM Duncan P. N. Exon Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: dexonsmith<br>Date: Mon Aug 26 11:29:51 2019<br>New Revision: 369943<br><br>URL:<span class="gmail-m_-2189833436213773647Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=369943&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=369943&view=rev</a><br>Log:<br>FileManager: Use llvm::Expected in new getFileRef API<br><br>`FileManager::getFileRef` is a modern API which we expect to convert to<br>over time.  We should modernize the error handling as well, using<br>`llvm::Expected` instead of `llvm::ErrorOr`, to help clients that care<br>about errors to ensure nothing is missed.<br><br>However, not all clients care.  I've also added another path for those<br>that don't:<br><br>- `FileEntryRef` is now copy- and move-assignable (using a pointer<br> <span class="gmail-m_-2189833436213773647Apple-converted-space"> </span>instead of a reference).<br>- `FileManager::getOptionalFileRef` returns an `llvm::Optional` instead<br> <span class="gmail-m_-2189833436213773647Apple-converted-space"> </span>of `llvm::Expected`.<br>- Added an `llvm::expectedToOptional` utility in case this is useful<br> <span class="gmail-m_-2189833436213773647Apple-converted-space"> </span>elsewhere.<br></blockquote><div><br>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.<br><br>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?</div></div></div></blockquote><div><br></div><div>`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.</div></div></div></blockquote><div><br>Fair enough - thanks for the context :)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div>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.<br></div></div></blockquote><div><br>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! <br><br>- Dave<br> </div></div></div>