[llvm] 27181ca - Support: Add Expected<T>::moveInto() to avoid extra names
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 22 11:47:22 PDT 2021
Author: Duncan P. N. Exon Smith
Date: 2021-10-22T11:47:03-07:00
New Revision: 27181cad0d4e48eac822aaccf52b612f29eeff73
URL: https://github.com/llvm/llvm-project/commit/27181cad0d4e48eac822aaccf52b612f29eeff73
DIFF: https://github.com/llvm/llvm-project/commit/27181cad0d4e48eac822aaccf52b612f29eeff73.diff
LOG: Support: Add Expected<T>::moveInto() to avoid extra names
Expected<T>::moveInto() takes as an out parameter any `OtherT&` that's
assignable from `T&&`. It moves any stored value before returning
takeError().
Since moveInto() consumes both the Error and the value, it's only
anticipated that we'd use call it on temporaries/rvalues, with naming
the Expected first likely to be an anti-pattern of sorts (either you
want to deal with both at the same time, or you don't). As such,
starting it out as `&&`-qualified... but it'd probably be fine to drop
that if there's a good use case for lvalues that appears.
There are two common patterns that moveInto() cleans up:
```
// If the variable is new:
Expected<std::unique_ptr<int>> ExpectedP = makePointer();
if (!ExpectedP)
return ExpectedP.takeError();
std::unique_ptr<int> P = std::move(*ExpectedP);
// If the target variable already exists:
if (Expected<T> ExpectedP = makePointer())
P = std::move(*ExpectedP);
else
return ExpectedP.takeError();
```
moveInto() takes less typing and avoids needing to name (or leak into
the scope) an extra variable.
```
// If the variable is new:
std::unique_ptr<int> P;
if (Error E = makePointer().moveInto(P))
return E;
// If the target variable already exists:
if (Error E = makePointer().moveInto(P))
return E;
```
It also seems useful for unit tests, to log errors (but continue) when
there's an unexpected failure. E.g.:
```
// Crash on error, or undefined in non-asserts builds.
std::unique_ptr<MemoryBuffer> MB = cantFail(makeMemoryBuffer());
// Avoid crashing on error without moveInto() :(.
Expected<std::unique_ptr<MemoryBuffer>>
ExpectedMB = makeMemoryBuffer();
ASSERT_THAT_ERROR(ExpectedMB.takeError(), Succeeded());
std::unique_ptr<MemoryBuffer> MB = std::move(ExpectedMB);
// Avoid crashing on error with moveInto() :).
std::unique_ptr<MemoryBuffer> MB;
ASSERT_THAT_ERROR(makeMemoryBuffer().moveInto(MB), Succeeded());
```
Differential Revision: https://reviews.llvm.org/D112278
Added:
Modified:
llvm/docs/ProgrammersManual.rst
llvm/include/llvm/Support/Error.h
llvm/unittests/Support/ErrorTest.cpp
Removed:
################################################################################
diff --git a/llvm/docs/ProgrammersManual.rst b/llvm/docs/ProgrammersManual.rst
index c10629d19e50..b3004af95e1c 100644
--- a/llvm/docs/ProgrammersManual.rst
+++ b/llvm/docs/ProgrammersManual.rst
@@ -568,6 +568,46 @@ rewritten as:
This second form is often more readable for functions that involve multiple
``Expected<T>`` values as it limits the indentation required.
+If an ``Expected<T>`` value will be moved into an existing variable then the
+``moveInto()`` method avoids the need to name an extra variable. This is
+useful to enable ``operator->()`` the ``Expected<T>`` value has pointer-like
+semantics. For example:
+
+.. code-block:: c++
+
+ Expected<std::unique_ptr<MemoryBuffer>> openBuffer(StringRef Path);
+ Error processBuffer(StringRef Buffer);
+
+ Error processBufferAtPath(StringRef Path) {
+ // Try to open a buffer.
+ std::unique_ptr<MemoryBuffer> MB;
+ if (auto Err = openBuffer(Path).moveInto(MB))
+ // On error, return the Error value.
+ return Err;
+ // On success, use MB.
+ return processContent(MB->getBuffer());
+ }
+
+This third form works with any type that can be assigned to from ``T&&``. This
+can be useful if the ``Expected<T>`` value needs to be stored an already-declared
+``Optional<T>``. For example:
+
+.. code-block:: c++
+
+ Expected<StringRef> extractClassName(StringRef Definition);
+ struct ClassData {
+ StringRef Definition;
+ Optional<StringRef> LazyName;
+ ...
+ Error initialize() {
+ if (auto Err = extractClassName(Path).moveInto(LazyName))
+ // On error, return the Error value.
+ return Err;
+ // On success, LazyName has been initialized.
+ ...
+ }
+ };
+
All ``Error`` instances, whether success or failure, must be either checked or
moved from (via ``std::move`` or a return) before they are destructed.
Accidentally discarding an unchecked error will cause a program abort at the
diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index 1e7201ed1b09..e2002b89ada2 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -577,6 +577,16 @@ template <class T> class LLVM_NODISCARD Expected {
return const_cast<Expected<T> *>(this)->get();
}
+ /// Returns \a takeError() after moving the held T (if any) into \p V.
+ template <class OtherT>
+ Error moveInto(OtherT &Value,
+ std::enable_if_t<std::is_assignable<OtherT &, T &&>::value> * =
+ nullptr) && {
+ if (*this)
+ Value = std::move(get());
+ return takeError();
+ }
+
/// Check that this Expected<T> is an error of type ErrT.
template <typename ErrT> bool errorIsA() const {
return HasError && (*getErrorStorage())->template isA<ErrT>();
diff --git a/llvm/unittests/Support/ErrorTest.cpp b/llvm/unittests/Support/ErrorTest.cpp
index 205f523f8a51..4d557cc7cbb0 100644
--- a/llvm/unittests/Support/ErrorTest.cpp
+++ b/llvm/unittests/Support/ErrorTest.cpp
@@ -1032,4 +1032,71 @@ TEST(Error, SubtypeStringErrorTest) {
"Error 2.");
}
+static Error createAnyError() {
+ return errorCodeToError(test_error_code::unspecified);
+}
+
+struct MoveOnlyBox {
+ Optional<int> Box;
+
+ explicit MoveOnlyBox(int I) : Box(I) {}
+ MoveOnlyBox() = default;
+ MoveOnlyBox(MoveOnlyBox &&) = default;
+ MoveOnlyBox &operator=(MoveOnlyBox &&) = default;
+
+ MoveOnlyBox(const MoveOnlyBox &) = delete;
+ MoveOnlyBox &operator=(const MoveOnlyBox &) = delete;
+
+ bool operator==(const MoveOnlyBox &RHS) const {
+ if (bool(Box) != bool(RHS.Box))
+ return false;
+ return Box ? *Box == *RHS.Box : false;
+ }
+};
+
+TEST(Error, moveInto) {
+ // Use MoveOnlyBox as the T in Expected<T>.
+ auto make = [](int I) -> Expected<MoveOnlyBox> { return MoveOnlyBox(I); };
+ auto makeFailure = []() -> Expected<MoveOnlyBox> { return createAnyError(); };
+
+ {
+ MoveOnlyBox V;
+
+ // Failure with no prior value.
+ EXPECT_THAT_ERROR(makeFailure().moveInto(V), Failed());
+ EXPECT_EQ(None, V.Box);
+
+ // Success with no prior value.
+ EXPECT_THAT_ERROR(make(5).moveInto(V), Succeeded());
+ EXPECT_EQ(5, V.Box);
+
+ // Success with an existing value.
+ EXPECT_THAT_ERROR(make(7).moveInto(V), Succeeded());
+ EXPECT_EQ(7, V.Box);
+
+ // Failure with an existing value. Might be nice to assign a
+ // default-constructed value in this case, but for now it's being left
+ // alone.
+ EXPECT_THAT_ERROR(makeFailure().moveInto(V), Failed());
+ EXPECT_EQ(7, V.Box);
+ }
+
+ // Check that this works with optionals too.
+ {
+ // Same cases as above.
+ Optional<MoveOnlyBox> MaybeV;
+ EXPECT_THAT_ERROR(makeFailure().moveInto(MaybeV), Failed());
+ EXPECT_EQ(None, MaybeV);
+
+ EXPECT_THAT_ERROR(make(5).moveInto(MaybeV), Succeeded());
+ EXPECT_EQ(MoveOnlyBox(5), MaybeV);
+
+ EXPECT_THAT_ERROR(make(7).moveInto(MaybeV), Succeeded());
+ EXPECT_EQ(MoveOnlyBox(7), MaybeV);
+
+ EXPECT_THAT_ERROR(makeFailure().moveInto(MaybeV), Failed());
+ EXPECT_EQ(MoveOnlyBox(7), MaybeV);
+ }
+}
+
} // namespace
More information about the llvm-commits
mailing list