<div dir="ltr">Ah, I see what I was missing - boolean testing a failed Error doesn't raise the checked flag. Carry on.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 25, 2016 at 8:27 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>That will assert-fail - there's a unit test for it.</div><div><br></div><div>Just so we've got our nomenclature clear:</div><div><br></div><div>(1) The checked bit is true if the error has been checked, and false otherwise. We want it to be false after the constructor exits to force clients to check the result.</div><div><br></div><div>(2) Boolean conversion for Error is: Error -> true, Success -> false.</div><div><br></div><div>So the ErrorAsOutParameter destructor is doing the right thing: If the user's constructor assigned an error at any point the checked flag will have been cleared, and the ErrorAsOutParameter destructor will not set it (boolean conversion doesn't set the flag if there's an error). If the user's constructor did not assign an error (i.e. !Err is true) then the ErrorAsOutParameter destructor will assign a fresh success value to Err, which will clear the flag.</div><div><br></div><div>Sometimes having a 'checked' flag feels a bit backwards - I wonder if having an 'unchecked' flag would actually be clearer - that way the flag value would mirror the boolean conversion operator result (true on error, false otherwise).</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>- Lang.</div><div><br></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 5:12 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"><div><div>On Fri, Mar 25, 2016 at 4:54 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 18:54:32 2016<br>
New Revision: 264479<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=264479&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=264479&view=rev</a><br>
Log:<br>
[Support] Switch to RAII helper for error-as-out-parameter idiom.<br>
<br>
As discussed on the llvm-commits thread for r264467.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/Error.h<br>
llvm/trunk/lib/Object/MachOObjectFile.cpp<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=264479&r1=264478&r2=264479&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=264479&r1=264478&r2=264479&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/Error.h (original)<br>
+++ llvm/trunk/include/llvm/Support/Error.h Fri Mar 25 18:54:32 2016<br>
@@ -143,13 +143,6 @@ 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>
@@ -552,6 +545,36 @@ inline void consumeError(Error Err) {<br>
handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});<br>
}<br>
<br>
+/// Helper for Errors used as out-parameters.<br>
+///<br>
+/// This helper is for use with the Error-as-out-parameter idiom, where an error<br>
+/// is passed to a function or method by reference, rather than being returned.<br>
+/// In such cases it is helpful to set the checked bit on entry to the function<br>
+/// so that the error can be written to (unchecked Errors abort on assignment)<br>
+/// and clear the checked bit on exit so that clients cannot accidentally forget<br>
+/// to check the result. This helper performs these actions automatically using<br>
+/// RAII:<br>
+///<br>
+/// Result foo(Error &Err) {<br>
+/// ErrorAsOutParameter ErrAsOutParam(Err); // 'Checked' flag set<br>
+/// // <body of foo><br>
+/// // <- 'Checked' flag auto-cleared when ErrAsOutParam is destructed.<br>
+/// }<br>
+class ErrorAsOutParameter {<br>
+public:<br>
+ ErrorAsOutParameter(Error &Err) : Err(Err) {<br>
+ // Raise the checked bit if Err is success.<br>
+ (void)!!Err;<br>
+ }<br>
+ ~ErrorAsOutParameter() {<br>
+ // Clear the checked bit.<br>
+ if (!Err)<br></blockquote></div></div><div><br>Wouldn't this raise the checked bit if there /was/ an error?<br><br>{ Error e; ErrorAsOutParameter(e); e = /* some actual error */; }<br><br>That should assert-fail, but I suspect it won't?</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Err = Error::success();<br>
+ }<br>
+private:<br>
+ Error &Err;<br>
+};<br>
+<br>
/// Tagged union holding either a T or a Error.<br>
///<br>
/// This class parallels ErrorOr, but replaces error_code with Error. Since<br>
<br>
Modified: llvm/trunk/lib/Object/MachOObjectFile.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=264479&r1=264478&r2=264479&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=264479&r1=264478&r2=264479&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Object/MachOObjectFile.cpp (original)<br>
+++ llvm/trunk/lib/Object/MachOObjectFile.cpp Fri Mar 25 18:54:32 2016<br>
@@ -246,7 +246,7 @@ static Error parseSegmentLoadCommand(<br>
Expected<std::unique_ptr<MachOObjectFile>><br>
MachOObjectFile::create(MemoryBufferRef Object, bool IsLittleEndian,<br>
bool Is64Bits) {<br>
- Error Err = Error::errorForOutParameter();<br>
+ Error Err;<br>
std::unique_ptr<MachOObjectFile> Obj(<br>
new MachOObjectFile(std::move(Object), IsLittleEndian,<br>
Is64Bits, Err));<br>
@@ -262,7 +262,7 @@ MachOObjectFile::MachOObjectFile(MemoryB<br>
DataInCodeLoadCmd(nullptr), LinkOptHintsLoadCmd(nullptr),<br>
DyldInfoLoadCmd(nullptr), UuidLoadCmd(nullptr),<br>
HasPageZeroSegment(false) {<br>
-<br>
+ ErrorAsOutParameter ErrAsOutParam(Err);<br>
if (is64Bit())<br>
parseHeader(this, Header64, Err);<br>
else<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=264479&r1=264478&r2=264479&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=264479&r1=264478&r2=264479&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/ErrorTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/ErrorTest.cpp Fri Mar 25 18:54:32 2016<br>
@@ -105,12 +105,32 @@ 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>
+// ErrorAsOutParameter tester.<br>
+void errAsOutParamHelper(Error &Err) {<br>
+ ErrorAsOutParameter ErrAsOutParam(Err);<br>
+ // Verify that checked flag is raised - assignment should not crash.<br>
+ Err = Error::success();<br>
+ // Raise the checked bit manually - caller should still have to test the<br>
+ // error.<br>
+ (void)!!Err;<br>
}<br>
<br>
+// Test that ErrorAsOutParameter sets the checked flag on construction.<br>
+TEST(Error, ErrorAsOutParameterChecked) {<br>
+ Error E;<br>
+ errAsOutParamHelper(E);<br>
+ (void)!!E;<br>
+}<br>
+<br>
+// Test that ErrorAsOutParameter clears the checked flag on destruction.<br>
+#ifndef NDEBUG<br>
+TEST(Error, ErrorAsOutParameterUnchecked) {<br>
+ EXPECT_DEATH({ Error E; errAsOutParamHelper(E); },<br>
+ "Program aborted due to an unhandled Error:")<br>
+ << "ErrorAsOutParameter did not clear the checked flag on destruction.";<br>
+}<br>
+#endif<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><br></div>