[llvm] r265446 - [Support] Add a checked flag to Expected<T>, require checks before access or
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 5 12:57:03 PDT 2016
Author: lhames
Date: Tue Apr 5 14:57:03 2016
New Revision: 265446
URL: http://llvm.org/viewvc/llvm-project?rev=265446&view=rev
Log:
[Support] Add a checked flag to Expected<T>, require checks before access or
destruction.
This makes the Expected<T> class behave like Error, even when in success mode.
Expected<T> values must be checked to see whether they contain an error prior
to being dereferenced, assigned to, or destructed.
Modified:
llvm/trunk/include/llvm/Support/Error.h
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=265446&r1=265445&r2=265446&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/Error.h (original)
+++ llvm/trunk/include/llvm/Support/Error.h Tue Apr 5 14:57:03 2016
@@ -131,6 +131,10 @@ class Error {
template <typename... HandlerTs>
friend Error handleErrors(Error E, HandlerTs &&... Handlers);
+ // Expected<T> needs to be able to steal the payload when constructed from an
+ // error.
+ template <typename T> class Expected;
+
public:
/// Create a success value. Prefer using 'Error::success()' for readability
/// where possible.
@@ -587,6 +591,8 @@ template <class T> class Expected {
static const bool isRef = std::is_reference<T>::value;
typedef ReferenceStorage<typename std::remove_reference<T>::type> wrap;
+ typedef std::unique_ptr<ErrorInfoBase> error_type;
+
public:
typedef typename std::conditional<isRef, wrap, T>::type storage_type;
@@ -598,8 +604,14 @@ private:
public:
/// Create an Expected<T> error value from the given Error.
- Expected(Error Err) : HasError(true) {
- assert(Err && "Cannot create Expected from Error success value.");
+ Expected(Error Err)
+ : HasError(true)
+#ifndef NDEBUG
+ ,
+ Checked(false)
+#endif
+ {
+ assert(Err && "Cannot create Expected<T> from Error success value.");
new (getErrorStorage()) Error(std::move(Err));
}
@@ -609,7 +621,12 @@ public:
Expected(OtherT &&Val,
typename std::enable_if<std::is_convertible<OtherT, T>::value>::type
* = nullptr)
- : HasError(false) {
+ : HasError(false)
+#ifndef NDEBUG
+ ,
+ Checked(false)
+#endif
+ {
new (getStorage()) storage_type(std::move(Val));
}
@@ -643,20 +660,32 @@ public:
/// Destroy an Expected<T>.
~Expected() {
+ assertIsChecked();
if (!HasError)
getStorage()->~storage_type();
else
- getErrorStorage()->~Error();
+ getErrorStorage()->~error_type();
}
/// \brief Return false if there is an error.
- explicit operator bool() const { return !HasError; }
+ explicit operator bool() {
+#ifndef NDEBUG
+ Checked = !HasError;
+#endif
+ return !HasError;
+ }
/// \brief Returns a reference to the stored T value.
- reference get() { return *getStorage(); }
+ reference get() {
+ assertIsChecked();
+ return *getStorage();
+ }
/// \brief Returns a const reference to the stored T value.
- const_reference get() const { return const_cast<Expected<T> *>(this)->get(); }
+ const_reference get() const {
+ assertIsChecked();
+ return const_cast<Expected<T> *>(this)->get();
+ }
/// \brief Check that this Expected<T> is an error of type ErrT.
template <typename ErrT> bool errorIsA() const {
@@ -668,20 +697,35 @@ public:
/// only be safely destructed. No further calls (beside the destructor) should
/// be made on the Expected<T> vaule.
Error takeError() {
- return HasError ? std::move(*getErrorStorage()) : Error();
+#ifndef NDEBUG
+ Checked = true;
+#endif
+ return HasError ? Error(std::move(*getErrorStorage())) : Error::success();
}
/// \brief Returns a pointer to the stored T value.
- pointer operator->() { return toPointer(getStorage()); }
+ pointer operator->() {
+ assertIsChecked();
+ return toPointer(getStorage());
+ }
/// \brief Returns a const pointer to the stored T value.
- const_pointer operator->() const { return toPointer(getStorage()); }
+ const_pointer operator->() const {
+ assertIsChecked();
+ return toPointer(getStorage());
+ }
/// \brief Returns a reference to the stored T value.
- reference operator*() { return *getStorage(); }
+ reference operator*() {
+ assertIsChecked();
+ return *getStorage();
+ }
/// \brief Returns a const reference to the stored T value.
- const_reference operator*() const { return *getStorage(); }
+ const_reference operator*() const {
+ assertIsChecked();
+ return *getStorage();
+ }
private:
template <class T1>
@@ -695,18 +739,22 @@ private:
}
template <class OtherT> void moveConstruct(Expected<OtherT> &&Other) {
- if (!Other.HasError) {
- // Get the other value.
- HasError = false;
+ HasError = Other.HasError;
+
+#ifndef NDEBUG
+ Checked = false;
+ Other.Checked = true;
+#endif
+
+ if (!HasError)
new (getStorage()) storage_type(std::move(*Other.getStorage()));
- } else {
- // Get other's error.
- HasError = true;
- new (getErrorStorage()) Error(Other.takeError());
- }
+ else
+ new (getErrorStorage()) error_type(std::move(*Other.getErrorStorage()));
}
template <class OtherT> void moveAssign(Expected<OtherT> &&Other) {
+ assertIsChecked();
+
if (compareThisIfSameType(*this, Other))
return;
@@ -732,16 +780,35 @@ private:
return reinterpret_cast<const storage_type *>(TStorage.buffer);
}
- Error *getErrorStorage() {
+ error_type *getErrorStorage() {
assert(HasError && "Cannot get error when a value exists!");
- return reinterpret_cast<Error *>(ErrorStorage.buffer);
+ return reinterpret_cast<error_type *>(ErrorStorage.buffer);
+ }
+
+ void assertIsChecked() {
+#ifndef NDEBUG
+ if (!Checked) {
+ dbgs() << "Expected<T> must be checked before access or destruction.\n";
+ if (HasError) {
+ dbgs() << "Unchecked Expected<T> contained error:\n";
+ (*getErrorStorage())->log(dbgs());
+ } else
+ dbgs() << "Expected<T> value was in success state. (Note: Expected<T> "
+ "values in success mode must still be checked prior to being "
+ "destroyed).\n";
+ abort();
+ }
+#endif
}
union {
AlignedCharArrayUnion<storage_type> TStorage;
- AlignedCharArrayUnion<Error> ErrorStorage;
+ AlignedCharArrayUnion<error_type> ErrorStorage;
};
bool HasError : 1;
+#ifndef NDEBUG
+ bool Checked : 1;
+#endif
};
/// This class wraps a std::error_code in a Error.
Modified: llvm/trunk/unittests/Support/ErrorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=265446&r1=265445&r2=265446&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/ErrorTest.cpp (original)
+++ llvm/trunk/unittests/Support/ErrorTest.cpp Tue Apr 5 14:57:03 2016
@@ -401,13 +401,47 @@ TEST(Error, ExitOnError) {
<< "exitOnError returned an unexpected error result";
}
-// Test Expected<T> in success mode.
-TEST(Error, ExpectedInSuccessMode) {
+// Test Checked Expected<T> in success mode.
+TEST(Error, CheckedExpectedInSuccessMode) {
Expected<int> A = 7;
EXPECT_TRUE(!!A) << "Expected with non-error value doesn't convert to 'true'";
+ // Access is safe in second test, since we checked the error in the first.
EXPECT_EQ(*A, 7) << "Incorrect Expected non-error value";
}
+// Test Unchecked Expected<T> in success mode.
+// We expect this to blow up the same way Error would.
+// Test runs in debug mode only.
+#ifndef NDEBUG
+TEST(Error, UncheckedExpectedInSuccessModeDestruction) {
+ EXPECT_DEATH({ Expected<int> A = 7; },
+ "Expected<T> must be checked before access or destruction.")
+ << "Unchecekd Expected<T> success value did not cause an abort().";
+}
+#endif
+
+// Test Unchecked Expected<T> in success mode.
+// We expect this to blow up the same way Error would.
+// Test runs in debug mode only.
+#ifndef NDEBUG
+TEST(Error, UncheckedExpectedInSuccessModeAccess) {
+ EXPECT_DEATH({ Expected<int> A = 7; *A; },
+ "Expected<T> must be checked before access or destruction.")
+ << "Unchecekd Expected<T> success value did not cause an abort().";
+}
+#endif
+
+// Test Unchecked Expected<T> in success mode.
+// We expect this to blow up the same way Error would.
+// Test runs in debug mode only.
+#ifndef NDEBUG
+TEST(Error, UncheckedExpectedInSuccessModeAssignment) {
+ EXPECT_DEATH({ Expected<int> A = 7; A = 7; },
+ "Expected<T> must be checked before access or destruction.")
+ << "Unchecekd Expected<T> success value did not cause an abort().";
+}
+#endif
+
// Test Expected<T> in failure mode.
TEST(Error, ExpectedInFailureMode) {
Expected<int> A = make_error<CustomError>(42);
@@ -423,7 +457,7 @@ TEST(Error, ExpectedInFailureMode) {
#ifndef NDEBUG
TEST(Error, AccessExpectedInFailureMode) {
Expected<int> A = make_error<CustomError>(42);
- EXPECT_DEATH(*A, "!HasError && \"Cannot get value when an error exists!\"")
+ EXPECT_DEATH(*A, "Expected<T> must be checked before access or destruction.")
<< "Incorrect Expected error value";
consumeError(A.takeError());
}
@@ -435,7 +469,7 @@ TEST(Error, AccessExpectedInFailureMode)
#ifndef NDEBUG
TEST(Error, UnhandledExpectedInFailureMode) {
EXPECT_DEATH({ Expected<int> A = make_error<CustomError>(42); },
- "Program aborted due to an unhandled Error:")
+ "Expected<T> must be checked before access or destruction.")
<< "Unchecked Expected<T> failure value did not cause an abort()";
}
#endif
@@ -446,10 +480,18 @@ TEST(Error, ExpectedCovariance) {
class D : public B {};
Expected<B *> A1(Expected<D *>(nullptr));
+ // Check A1 by converting to bool before assigning to it.
+ (void)!!A1;
A1 = Expected<D *>(nullptr);
+ // Check A1 again before destruction.
+ (void)!!A1;
Expected<std::unique_ptr<B>> A2(Expected<std::unique_ptr<D>>(nullptr));
+ // Check A2 by converting to bool before assigning to it.
+ (void)!!A2;
A2 = Expected<std::unique_ptr<D>>(nullptr);
+ // Check A2 again before destruction.
+ (void)!!A2;
}
TEST(Error, ErrorCodeConversions) {
More information about the llvm-commits
mailing list