<div dir="ltr">Hi Dave,<div><br></div><div>The constructor should assign to Error no matter what (I may have failed to assign success in the MachOObjectFile constructor - I'll check), and consequently the caller will have to check no matter what. The issue is with the assignment operator. It's an error to overwrite an unchecked value, so the following will fail:</div><div><br></div><div>Error Err = Error::success();</div><div>Err = Error::success(); // <- Failure to check the first success value.</div><div><br></div><div>Using</div><div><br></div><div>Error = Err::errorForOutParameter();</div><div>Error = Error::success();</div><div><br></div><div>Instead is safe.</div><div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 25, 2016 at 3:05 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Fri, Mar 25, 2016 at 2:56 PM, Lang Hames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: lhames<br>
Date: Fri Mar 25 16:56:35 2016<br>
New Revision: 264467<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=264467&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=264467&view=rev</a><br>
Log:<br>
[Support] Add Error::errorForOutParameter helper.<br>
<br>
This helper method creates a pre-checked Error suitable for use as an out<br>
parameter in a constructor. This avoids the need to have the constructor<br>
check a known-good error before assigning to it.<br></blockquote><div><br></div></span><div>Hmm, thinking about it - how does this work to ensure safety?<br><br>If the callee only assigns to the parameter on failure cases, won't the code fail to assert on successful cases?<br><br>(ie: shouldn't it actually be unchecked by default as it was before - the caller should check it on return regardless of what the function did)</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/Error.h<br>
llvm/trunk/unittests/Support/ErrorTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/Error.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=264467&r1=264466&r2=264467&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=264467&r1=264466&r2=264467&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/Error.h (original)<br>
+++ llvm/trunk/include/llvm/Support/Error.h Fri Mar 25 16:56:35 2016<br>
@@ -143,6 +143,13 @@ public:<br>
/// constructor, but should be preferred for readability where possible.<br>
static Error success() { return Error(); }<br>
<br>
+ /// Create a 'pre-checked' success value suitable for use as an out-parameter.<br>
+ static Error errorForOutParameter() {<br>
+ Error Err;<br>
+ (void)!!Err;<br>
+ return Err;<br>
+ }<br>
+<br>
// Errors are not copy-constructable.<br>
Error(const Error &Other) = delete;<br>
<br>
<br>
Modified: llvm/trunk/unittests/Support/ErrorTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=264467&r1=264466&r2=264467&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=264467&r1=264466&r2=264467&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/ErrorTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/ErrorTest.cpp Fri Mar 25 16:56:35 2016<br>
@@ -105,6 +105,12 @@ TEST(Error, UncheckedSuccess) {<br>
}<br>
#endif<br>
<br>
+// Test that errors to be used as out parameters are implicitly checked (<br>
+// and thus destruct quietly).<br>
+TEST(Error, ErrorAsOutParameter) {<br>
+ Error E = Error::errorForOutParameter();<br>
+}<br>
+<br>
// Check that we abort on unhandled failure cases. (Force conversion to bool<br>
// to make sure that we don't accidentally treat checked errors as handled).<br>
// Test runs in debug mode only.<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>