[llvm] r264479 - [Support] Switch to RAII helper for error-as-out-parameter idiom.

Fri Mar 25 16:54:32 PDT 2016

Author: lhames
Date: Fri Mar 25 18:54:32 2016
New Revision: 264479

URL: http://llvm.org/viewvc/llvm-project?rev=264479&view=rev
Log:
[Support] Switch to RAII helper for error-as-out-parameter idiom.

As discussed on the llvm-commits thread for r264467.

Modified:
    llvm/trunk/include/llvm/Support/Error.h
    llvm/trunk/lib/Object/MachOObjectFile.cpp
    llvm/trunk/unittests/Support/ErrorTest.cpp

Modified: llvm/trunk/include/llvm/Support/Error.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=264479&r1=264478&r2=264479&view=diff
==============================================================================

--- llvm/trunk/include/llvm/Support/Error.h (original)
+++ llvm/trunk/include/llvm/Support/Error.h Fri Mar 25 18:54:32 2016
@@ -143,13 +143,6 @@ public:
   /// constructor, but should be preferred for readability where possible.
   static Error success() { return Error(); }
 
-  /// Create a 'pre-checked' success value suitable for use as an out-parameter.
-  static Error errorForOutParameter() {
-    Error Err;
-    (void)!!Err;
-    return Err;
-  }
-
   // Errors are not copy-constructable.
   Error(const Error &Other) = delete;
 
@@ -552,6 +545,36 @@ inline void consumeError(Error Err) {
   handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
 }
 
+/// Helper for Errors used as out-parameters.
+///
+/// This helper is for use with the Error-as-out-parameter idiom, where an error
+/// is passed to a function or method by reference, rather than being returned.
+/// In such cases it is helpful to set the checked bit on entry to the function
+/// so that the error can be written to (unchecked Errors abort on assignment)
+/// and clear the checked bit on exit so that clients cannot accidentally forget
+/// to check the result. This helper performs these actions automatically using
+/// RAII:
+///
+/// Result foo(Error &Err) {
+///   ErrorAsOutParameter ErrAsOutParam(Err); // 'Checked' flag set
+///   // <body of foo>
+///   // <- 'Checked' flag auto-cleared when ErrAsOutParam is destructed.
+/// }
+class ErrorAsOutParameter {
+public:
+  ErrorAsOutParameter(Error &Err) : Err(Err) {
+    // Raise the checked bit if Err is success.
+    (void)!!Err;
+  }
+  ~ErrorAsOutParameter() {
+    // Clear the checked bit.
+    if (!Err)
+      Err = Error::success();
+  }
+private:
+  Error &Err;
+};
+
 /// Tagged union holding either a T or a Error.
 ///
 /// This class parallels ErrorOr, but replaces error_code with Error. Since

Modified: llvm/trunk/lib/Object/MachOObjectFile.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=264479&r1=264478&r2=264479&view=diff
==============================================================================
--- llvm/trunk/lib/Object/MachOObjectFile.cpp (original)
+++ llvm/trunk/lib/Object/MachOObjectFile.cpp Fri Mar 25 18:54:32 2016
@@ -246,7 +246,7 @@ static Error parseSegmentLoadCommand(
 Expected<std::unique_ptr<MachOObjectFile>>
 MachOObjectFile::create(MemoryBufferRef Object, bool IsLittleEndian,
                         bool Is64Bits) {
-  Error Err = Error::errorForOutParameter();
+  Error Err;
   std::unique_ptr<MachOObjectFile> Obj(
       new MachOObjectFile(std::move(Object), IsLittleEndian,
                            Is64Bits, Err));
@@ -262,7 +262,7 @@ MachOObjectFile::MachOObjectFile(MemoryB
       DataInCodeLoadCmd(nullptr), LinkOptHintsLoadCmd(nullptr),
       DyldInfoLoadCmd(nullptr), UuidLoadCmd(nullptr),
       HasPageZeroSegment(false) {
-
+  ErrorAsOutParameter ErrAsOutParam(Err);
   if (is64Bit())
     parseHeader(this, Header64, Err);
   else

Modified: llvm/trunk/unittests/Support/ErrorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=264479&r1=264478&r2=264479&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/ErrorTest.cpp (original)
+++ llvm/trunk/unittests/Support/ErrorTest.cpp Fri Mar 25 18:54:32 2016
@@ -105,12 +105,32 @@ TEST(Error, UncheckedSuccess) {
 }
 #endif
 
-// Test that errors to be used as out parameters are implicitly checked (
-// and thus destruct quietly).
-TEST(Error, ErrorAsOutParameter) {
-  Error E = Error::errorForOutParameter();
+// ErrorAsOutParameter tester.
+void errAsOutParamHelper(Error &Err) {
+  ErrorAsOutParameter ErrAsOutParam(Err);
+  // Verify that checked flag is raised - assignment should not crash.
+  Err = Error::success();
+  // Raise the checked bit manually - caller should still have to test the
+  // error.
+  (void)!!Err;
 }
 
+// Test that ErrorAsOutParameter sets the checked flag on construction.
+TEST(Error, ErrorAsOutParameterChecked) {
+  Error E;
+  errAsOutParamHelper(E);
+  (void)!!E;
+}
+
+// Test that ErrorAsOutParameter clears the checked flag on destruction.
+#ifndef NDEBUG
+TEST(Error, ErrorAsOutParameterUnchecked) {
+  EXPECT_DEATH({ Error E; errAsOutParamHelper(E); },
+               "Program aborted due to an unhandled Error:")
+      << "ErrorAsOutParameter did not clear the checked flag on destruction.";
+}
+#endif
+
 // Check that we abort on unhandled failure cases. (Force conversion to bool
 // to make sure that we don't accidentally treat checked errors as handled).
 // Test runs in debug mode only.


