From d1af8fce0fa1d05d635eeb892b82f2cca1cee1cd Mon Sep 17 00:00:00 2001 From: Lang Hames Date: Fri, 25 Mar 2016 23:54:32 +0000 Subject: [PATCH] [Support] Switch to RAII helper for error-as-out-parameter idiom. As discussed on the llvm-commits thread for r264467. llvm-svn: 264479 --- llvm/include/llvm/Support/Error.h | 37 +++++++++++++++++++++++++++++------- llvm/lib/Object/MachOObjectFile.cpp | 4 ++-- llvm/unittests/Support/ErrorTest.cpp | 28 +++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h index f96e02c..304925c 100644 --- a/llvm/include/llvm/Support/Error.h +++ b/llvm/include/llvm/Support/Error.h @@ -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 +/// // +/// // <- '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 diff --git a/llvm/lib/Object/MachOObjectFile.cpp b/llvm/lib/Object/MachOObjectFile.cpp index 864d76f..607c246 100644 --- a/llvm/lib/Object/MachOObjectFile.cpp +++ b/llvm/lib/Object/MachOObjectFile.cpp @@ -246,7 +246,7 @@ static Error parseSegmentLoadCommand( Expected> MachOObjectFile::create(MemoryBufferRef Object, bool IsLittleEndian, bool Is64Bits) { - Error Err = Error::errorForOutParameter(); + Error Err; std::unique_ptr Obj( new MachOObjectFile(std::move(Object), IsLittleEndian, Is64Bits, Err)); @@ -262,7 +262,7 @@ MachOObjectFile::MachOObjectFile(MemoryBufferRef Object, bool IsLittleEndian, DataInCodeLoadCmd(nullptr), LinkOptHintsLoadCmd(nullptr), DyldInfoLoadCmd(nullptr), UuidLoadCmd(nullptr), HasPageZeroSegment(false) { - + ErrorAsOutParameter ErrAsOutParam(Err); if (is64Bit()) parseHeader(this, Header64, Err); else diff --git a/llvm/unittests/Support/ErrorTest.cpp b/llvm/unittests/Support/ErrorTest.cpp index 1a0dd44..7d71477 100644 --- a/llvm/unittests/Support/ErrorTest.cpp +++ b/llvm/unittests/Support/ErrorTest.cpp @@ -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. -- 2.7.4