[llvm] r286562 - Prevent at compile time converting from Error::success() to Expected<T>
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 10 20:29:28 PST 2016
Author: mehdi_amini
Date: Thu Nov 10 22:29:25 2016
New Revision: 286562
URL: http://llvm.org/viewvc/llvm-project?rev=286562&view=rev
Log:
Prevent at compile time converting from Error::success() to Expected<T>
This would trigger an assertion at runtime otherwise.
Differential Revision: https://reviews.llvm.org/D26482
Modified:
llvm/trunk/include/llvm/Support/Error.h
llvm/trunk/lib/Bitcode/Reader/BitcodeReader.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=286562&r1=286561&r2=286562&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/Error.h (original)
+++ llvm/trunk/include/llvm/Support/Error.h Thu Nov 10 22:29:25 2016
@@ -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<T> 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 <typename ErrT, typename... ArgTs> 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<T> foo() { return Error::success(); } which compiles otherwise
+ /// but triggers the assertion above.
+ Expected(ErrorSuccess) = delete;
+
/// Create an Expected<T> success value from the given OtherT value, which
/// must be convertible to T.
template <typename OtherT>
Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=286562&r1=286561&r2=286562&view=diff
==============================================================================
--- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
+++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Thu Nov 10 22:29:25 2016
@@ -4330,7 +4330,7 @@ Expected<std::string> BitcodeReader::par
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<std::string> BitcodeReader::par
// 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<bool> BitcodeReader::hasObjCCat
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)
Modified: llvm/trunk/unittests/Support/ErrorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=286562&r1=286561&r2=286562&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/ErrorTest.cpp (original)
+++ llvm/trunk/unittests/Support/ErrorTest.cpp Thu Nov 10 22:29:25 2016
@@ -97,14 +97,15 @@ static void handleCustomErrorUPVoid(std:
// 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<CustomError>(42),
- [](const CustomError &CE) mutable { return Error::success(); });
+ [](const CustomError &CE) mutable -> Error { return Error::success(); });
handleAllErrors(make_error<CustomError>(42),
[](const CustomError &CE) mutable {});
handleAllErrors(make_error<CustomError>(42),
- [](CustomError &CE) { return Error::success(); });
+ [](CustomError &CE) -> Error { return Error::success(); });
handleAllErrors(make_error<CustomError>(42), [](CustomError &CE) {});
handleAllErrors(make_error<CustomError>(42),
- [](CustomError &CE) mutable { return Error::success(); });
+ [](CustomError &CE) mutable -> Error { return Error::success(); });
handleAllErrors(make_error<CustomError>(42), [](CustomError &CE) mutable {});
handleAllErrors(
make_error<CustomError>(42),
- [](std::unique_ptr<CustomError> CE) { return Error::success(); });
+ [](std::unique_ptr<CustomError> CE) -> Error { return Error::success(); });
handleAllErrors(make_error<CustomError>(42),
[](std::unique_ptr<CustomError> CE) {});
handleAllErrors(
make_error<CustomError>(42),
- [](std::unique_ptr<CustomError> CE) mutable { return Error::success(); });
+ [](std::unique_ptr<CustomError> CE) mutable -> Error { return Error::success(); });
handleAllErrors(make_error<CustomError>(42),
[](std::unique_ptr<CustomError> 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));
}
More information about the llvm-commits
mailing list