Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpp: Fix evmc::result's move assignment operator #282

Merged
merged 4 commits into from
May 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
## [6.2.2] - unreleased

- Fixed: [[#281](https://github.com/ethereum/evmc/pull/281)]
Compilation error of evmc::result::raw() in Visual Studio fixed.
Compilation error of `evmc::result::raw()` in Visual Studio fixed.
- Fixed: [[#282](https://github.com/ethereum/evmc/pull/282)]
The `evmc::result`'s move assignment operator fixed.

## [6.2.1] - 2019-04-29

Expand Down
18 changes: 11 additions & 7 deletions include/evmc/evmc.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,20 @@ class result : private evmc_result
/// Move constructor.
result(result&& other) noexcept : evmc_result{other}
{
// Disable releaser of the rvalue object.
other.release = nullptr;
other.release = nullptr; // Disable releasing of the rvalue object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you do also other.output_data = nullptr to avoid dangling other?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering zeroing the other, but using an object that has been moved is an error anyway. I decided not to because there are other cases which will be hard to cover. Fortunately, some compilers warn you about using an object after std::move().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For raw() there are no language rules though, and after calling raw() this contains dangling pointer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a number of things we can do about it.

  1. This is already documented.
  2. The function is rather internal, needed to the Host interface. We might hide it by making it private and friend with the Host.
  3. We can zero the other in raw(). It might be better than the current but still not protecting against using the object after raw() (it might actually hide the problem deeper) and that will set the .status_code to EVMC_SUCCESS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could zero out everything but status_code.

Anyway I don't think it's super-critical, having it documented might be enough.
(maybe I would name it release() to be similar to smart pointers)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

release()

I wanted that, but unfortunately evmc_result already has the release method. I wander how to rename it to make the usage more obvious. release_raw()? move_raw()? take_raw()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

release_raw sounds better to me than raw

}

result(result const&) = delete;

/// Move-and-swap assignment operator.
result& operator=(result other) noexcept
/// Move assignment operator.
///
/// The self-assigment MUST never happen.
///
/// @param other The other result object.
/// @return The reference to the left-hand side object.
result& operator=(result&& other) noexcept
{
std::swap(*this, other);
this->~result(); // Release this object.
static_cast<evmc_result&>(*this) = other; // Copy data.
other.release = nullptr; // Disable releasing of the rvalue object.
return *this;
}

Expand Down
37 changes: 37 additions & 0 deletions test/unittests/test_cpp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,40 @@ TEST(cpp, result_raii)
}
EXPECT_EQ(release_called, 1);
}

TEST(cpp, result_move)
{
static auto release_called = 0;
auto release_fn = [](const evmc_result*) noexcept { ++release_called; };

release_called = 0;
{
auto raw = evmc_result{};
raw.gas_left = -1;
raw.release = release_fn;

auto r0 = evmc::result{raw};
EXPECT_EQ(r0.gas_left, raw.gas_left);

auto r1 = std::move(r0);
EXPECT_EQ(r1.gas_left, raw.gas_left);
}
EXPECT_EQ(release_called, 1);

release_called = 0;
{
auto raw1 = evmc_result{};
raw1.gas_left = 1;
raw1.release = release_fn;

auto raw2 = evmc_result{};
raw2.gas_left = 1;
raw2.release = release_fn;

auto r1 = evmc::result{raw1};
auto r2 = evmc::result{raw2};

r2 = std::move(r1);
}
EXPECT_EQ(release_called, 2);
}