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

coroutine: experimental: generator: implement move and swap #2354

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bhalevy
Copy link
Member

@bhalevy bhalevy commented Jul 17, 2024

The coroutine generator move ctor, move assignment operator, and swap override were never fully implemented.

This patch completes their implementation
and adds unit tests to cover move and swap
and also moving not-drained generator (to test
moving of their buffered value(s)).

Fixes #1789

The coroutine generator move ctor, move assignment operator,
and swap override were never fully implemented.

This patch completes their implementation
and adds unit tests to cover move and swap
and also moving not-drained generator (to test
moving of their buffered value(s)).

Fixes scylladb#1789

Signed-off-by: Benny Halevy <[email protected]>
@bhalevy bhalevy force-pushed the fix-coroutine-generator-move branch from 1d7a876 to 311c33d Compare July 17, 2024 22:29
@tchaikov
Copy link
Contributor

@bhalevy hi Benny, thank you for addressing this issue. but i think the more fundamental problems of the generator are

I believe we should address these issues before fixing the move semantics of the generator. I agree that they are the next important matters to tackle. Perhaps you could help review pull request #2218? Once it's merged, we should be in a better position to resolve issue #1789. What are your thoughts on this approach?

@bhalevy
Copy link
Member Author

bhalevy commented Jul 19, 2024

@bhalevy hi Benny, thank you for addressing this issue. but i think the more fundamental problems of the generator are

I believe we should address these issues before fixing the move semantics of the generator. I agree that they are the next important matters to tackle. Perhaps you could help review pull request #2218? Once it's merged, we should be in a better position to resolve issue #1789. What are your thoughts on this approach?

I have no objection.
Just please add the additional tests in the PR to #2218 to thoroughly test move semantics of the async_generator.

@bhalevy
Copy link
Member Author

bhalevy commented Sep 23, 2024

@avikivity, @tchaikov: since #2218 is taking longer than expected,
how about merging this fix first and rebasing #2218 on top of it?

Cc @regevran

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

@bhalevy this change is definitely a valuable step forward. however, to make the generator truly production-ready, we'll still need to address the issues outlined above, either through #2218 or another solution.

@bhalevy
Copy link
Member Author

bhalevy commented Sep 24, 2024

@scylladb/seastar-maint please consider merging.
Although this PR is not the perfect solution it still provides net-positive benefits.

_promise->set_generator(this);
}
_values = std::move(other._values);
const_cast<size_t&>(_buffer_capacity) = other._buffer_capacity;

Choose a reason for hiding this comment

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

Maybe:

Suggested change
const_cast<size_t&>(_buffer_capacity) = other._buffer_capacity;
const_cast<size_t&>(_buffer_capacity) = std::exchange(other._buffer_capacity, 0);

@@ -768,6 +774,37 @@ seastar::future<> test_async_generator_drained() {
BOOST_REQUIRE(!sentinel.has_value());
}

template<template<typename> class Container>

Choose a reason for hiding this comment

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

The entire new section is missing some verbal explanations about the tests, the expected results and how they are achieved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coroutine generator cannot be moved or composed.
3 participants