From 5652444405b88fddf3eba4e86ba1f345d651fe8c Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Fri, 11 Nov 2016 04:29:25 +0000 Subject: [PATCH] Prevent at compile time converting from Error::success() to Expected This would trigger an assertion at runtime otherwise. Differential Revision: https://reviews.llvm.org/D26482 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@286562 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/Error.h | 18 +++++++++++++++--- lib/Bitcode/Reader/BitcodeReader.cpp | 8 ++++---- unittests/Support/ErrorTest.cpp | 21 +++++++++++---------- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/include/llvm/Support/Error.h b/include/llvm/Support/Error.h index e4f352ab95e..a09a212c12f 100644 --- a/include/llvm/Support/Error.h +++ b/include/llvm/Support/Error.h @@ -27,6 +27,7 @@ namespace llvm { class Error; class ErrorList; +class ErrorSuccess; /// Base class for error info classes. Do not extend this directly: Extend /// the ErrorInfo template subclass instead. @@ -157,9 +158,8 @@ protected: } public: - /// Create a success value. This is equivalent to calling the default - /// constructor, but should be preferred for readability where possible. - static Error success() { return Error(); } + /// Create a success value. + static ErrorSuccess success(); // Errors are not copy-constructable. Error(const Error &Other) = delete; @@ -279,6 +279,13 @@ private: ErrorInfoBase *Payload; }; +/// Subclass of Error for the sole purpose of identifying the success path in +/// the type system. This allows to catch invalid conversion to Expected at +/// compile time. +class ErrorSuccess : public Error {}; + +inline ErrorSuccess Error::success() { return ErrorSuccess(); } + /// Make a Error instance representing failure using the given error info /// type. template Error make_error(ArgTs &&... Args) { @@ -645,6 +652,11 @@ public: new (getErrorStorage()) error_type(Err.takePayload()); } + /// Forbid to convert from Error::success() implicitly, this avoids having + /// Expected foo() { return Error::success(); } which compiles otherwise + /// but triggers the assertion above. + Expected(ErrorSuccess) = delete; + /// Create an Expected success value from the given OtherT value, which /// must be convertible to T. template diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp index d2c2ec774c1..270a1c49997 100644 --- a/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/lib/Bitcode/Reader/BitcodeReader.cpp @@ -4330,7 +4330,7 @@ Expected BitcodeReader::parseTriple() { case BitstreamEntry::Error: return error("Malformed block"); case BitstreamEntry::EndBlock: - return Error::success(); + return ""; case BitstreamEntry::SubBlock: if (Entry.ID == bitc::MODULE_BLOCK_ID) @@ -4363,14 +4363,14 @@ Expected BitcodeReader::parseIdentificationBlock() { // we need to make sure we aren't at the end of the stream before calling // advance, otherwise we'll get an error. if (Stream.AtEndOfStream()) - return Error::success(); + return ""; BitstreamEntry Entry = Stream.advance(); switch (Entry.Kind) { case BitstreamEntry::Error: return error("Malformed block"); case BitstreamEntry::EndBlock: - return Error::success(); + return ""; case BitstreamEntry::SubBlock: if (Entry.ID == bitc::IDENTIFICATION_BLOCK_ID) { @@ -4421,7 +4421,7 @@ Expected BitcodeReader::hasObjCCategory() { case BitstreamEntry::Error: return error("Malformed block"); case BitstreamEntry::EndBlock: - return Error::success(); + return false; case BitstreamEntry::SubBlock: if (Entry.ID == bitc::MODULE_BLOCK_ID) diff --git a/unittests/Support/ErrorTest.cpp b/unittests/Support/ErrorTest.cpp index 7468a857b89..1fe901745ca 100644 --- a/unittests/Support/ErrorTest.cpp +++ b/unittests/Support/ErrorTest.cpp @@ -97,14 +97,15 @@ static void handleCustomErrorUPVoid(std::unique_ptr CE) {} // Test that success values implicitly convert to false, and don't cause crashes // once they've been implicitly converted. TEST(Error, CheckedSuccess) { - Error E; + Error E = Error::success(); EXPECT_FALSE(E) << "Unexpected error while testing Error 'Success'"; } // Test that unchecked succes values cause an abort. #ifndef NDEBUG TEST(Error, UncheckedSuccess) { - EXPECT_DEATH({ Error E; }, "Program aborted due to an unhandled Error:") + EXPECT_DEATH({ Error E = Error::success(); }, + "Program aborted due to an unhandled Error:") << "Unchecked Error Succes value did not cause abort()"; } #endif @@ -121,7 +122,7 @@ void errAsOutParamHelper(Error &Err) { // Test that ErrorAsOutParameter sets the checked flag on construction. TEST(Error, ErrorAsOutParameterChecked) { - Error E; + Error E = Error::success(); errAsOutParamHelper(E); (void)!!E; } @@ -129,7 +130,7 @@ TEST(Error, ErrorAsOutParameterChecked) { // Test that ErrorAsOutParameter clears the checked flag on destruction. #ifndef NDEBUG TEST(Error, ErrorAsOutParameterUnchecked) { - EXPECT_DEATH({ Error E; errAsOutParamHelper(E); }, + EXPECT_DEATH({ Error E = Error::success(); errAsOutParamHelper(E); }, "Program aborted due to an unhandled Error:") << "ErrorAsOutParameter did not clear the checked flag on destruction."; } @@ -197,31 +198,31 @@ TEST(Error, HandlerTypeDeduction) { handleAllErrors( make_error(42), - [](const CustomError &CE) mutable { return Error::success(); }); + [](const CustomError &CE) mutable -> Error { return Error::success(); }); handleAllErrors(make_error(42), [](const CustomError &CE) mutable {}); handleAllErrors(make_error(42), - [](CustomError &CE) { return Error::success(); }); + [](CustomError &CE) -> Error { return Error::success(); }); handleAllErrors(make_error(42), [](CustomError &CE) {}); handleAllErrors(make_error(42), - [](CustomError &CE) mutable { return Error::success(); }); + [](CustomError &CE) mutable -> Error { return Error::success(); }); handleAllErrors(make_error(42), [](CustomError &CE) mutable {}); handleAllErrors( make_error(42), - [](std::unique_ptr CE) { return Error::success(); }); + [](std::unique_ptr CE) -> Error { return Error::success(); }); handleAllErrors(make_error(42), [](std::unique_ptr CE) {}); handleAllErrors( make_error(42), - [](std::unique_ptr CE) mutable { return Error::success(); }); + [](std::unique_ptr CE) mutable -> Error { return Error::success(); }); handleAllErrors(make_error(42), [](std::unique_ptr CE) mutable {}); @@ -365,7 +366,7 @@ TEST(Error, CheckJoinErrors) { // Test that we can consume success values. TEST(Error, ConsumeSuccess) { - Error E; + Error E = Error::success(); consumeError(std::move(E)); } -- 2.40.0