<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><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><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">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><br></div></div>