<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 25, 2016 at 3:30 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@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">Actually, maybe RAII is the way to go here:<div><br></div><div>Instead of:</div><div><br></div><div><font face="monospace, monospace">Foo::Foo(Error &Err) {</font></div><div><font face="monospace, monospace"> // ...</font></div><div><font face="monospace, monospace"> if (Cond)</font></div><div><font face="monospace, monospace"> Err = ...</font></div><div><font face="monospace, monospace"> // ...</font></div><div><font face="monospace, monospace">}</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">Expected<Foo> Foo::create() {</font></div><div><font face="monospace, monospace"> Error Err = Error::errorForOutParameter();</font></div><div><font face="monospace, monospace"> Foo F(Err);</font></div><div><font face="monospace, monospace"> if (Err)</font></div><div><font face="monospace, monospace"> return std::move(Err);</font></div><div><font face="monospace, monospace"> return std::move(F);</font></div><div><font face="monospace, monospace">}</font></div><div><br></div><div>We could have:</div><div><br></div><div><div><font face="monospace, monospace">Foo::Foo(Error &Err) {</font></div><div><font face="monospace, monospace"> ErrorForOutParam _(Err); // '_' clears checked flag at entry, unclears it at exit.</font></div><div><font face="monospace, monospace"> // ...</font></div><div><font face="monospace, monospace"> if (Cond)</font></div><div><font face="monospace, monospace"> Err = ...</font></div><div><font face="monospace, monospace"> // ...</font></div><div><font face="monospace, monospace">}</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">Expected<Foo> Foo::create() {</font></div><div><font face="monospace, monospace"> Error Err;</font></div><div><font face="monospace, monospace"> Foo F(Err);</font></div><div><font face="monospace, monospace"> if (Err)</font></div><div><font face="monospace, monospace"> return std::move(Err);</font></div><div><font face="monospace, monospace"> return std::move(F);</font></div><div><font face="monospace, monospace">}</font></div></div><div><br></div><div>What do you think?</div></div></blockquote><div><br></div><div>Yeah, seems legit - & quick/easy enough to write (not a lot of engineering for a corner case), etc.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="HOEnZb"><font color="#888888"><div><br></div><div>- Lang.</div><div><br></div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 25, 2016 at 3:14 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>On Fri, Mar 25, 2016 at 3:11 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@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">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></blockquote><div><br></div></span><div>Right... hmm, tricky. :/ yeah, not sure of the best way to account for all of that & make it least error prone in these use cases.</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>- Lang.</div></font></span></div><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>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><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>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>