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

Explicit access to initial and final values of coroutine #68

Merged
merged 1 commit into from
Jul 7, 2016

Conversation

therustmonk
Copy link
Contributor

PR #67 is great, but it has one imperfection: impossible to specify return value when coroutine ends (it return usize::MAX by default. I use library dinamically and take segfaults, because resume expects valid value anytime. We can solve it if closure will return value explicitly, but it makes API quite ugly and forces users to rewrite more code.

Another ides I tried:

  • Check usize::MAX value, but I have to return additional data.
  • If I will use explicit yield on tail, coroutine will contains invalid is_finished status.

I've made this PR to solve mentioned problems in better way. This PR takes profits:

  1. Add possibility to get initial value of first resume call (by get_first method of Coroutine)
  2. Add possibility to set final value when coroutine ends to last resume call (by set_last method of Coroutine)
  3. Revert closure type to FnOnce(&mut Coroutine) to prevent user's code changing

It's more user-friendly, because API not changed and nobody have to change closures, but anyone can control first and last value explicitly.

@zonyitoo It needs your resoulition.

@zonyitoo
Copy link
Contributor

zonyitoo commented Jul 7, 2016

the first_data and last_data may be too confusing to users, is there another way to capture the "first data" and "last data"? Or we can find a way to avoid these.

@therustmonk
Copy link
Contributor Author

therustmonk commented Jul 7, 2016

There are some. Let's choose the better way:


First approach: Use get_first and set_last methods.

Usage:

let mut coro1 = Coroutine::spawn(move |me| {
    let initial = me.get_first();
    me.yield_with(1);
    me.set_last(2);
});

Pros:

  • Users have not to change own code

Cons:

  • We add two usize fields to struct

Second approach: Rename methods to something more meaningful (get_initial_data, set_final_data);

Usage + Pros + Cons are the same as in first.


Third approach: Use only one cross value.

Usage:

let mut coro1 = Coroutine::spawn(move |me| {
    let initial = me.get_cross();
    me.yield_with(1);
    me.set_cross(2);
});

Pros:

  • Users have not to change own code
  • We remove one field from struct

Cons:

  • We add one usize field to struct

Fourth approach: Change signature of closure from FnOnce(&mut Coroutine) to FnOnce(&mut Coroutine, usize) -> usize.

Usage:

let mut coro1 = Coroutine::spawn(move |me, initial| {
    me.yield_with(1);
    2
});

Pros:

  • No additional fields in struct
  • More explicit for users

Cons:

  • Users have to change some code
  • More verbose for users

Fifth approach: Wrap spawn with additional closure for spawn(FnOnce(&mut Coroutine)) and add new spanw_explicit(FnOnce(&mut Coroutine, usize) -> usize) method.

Usage:

let mut coro1 = Coroutine::spawn(move |me| {
    me.yield_with(1);
});
let mut coro2 = Coroutine::spawn_explicit(move |me, initial| {
    me.yield_with(1);
    2
});

// In fact `spawn` wraps it like:
fn spawn(f:F) {
    spawn_explicit(|me, _| {
        f(me);
        usize:MAX
    })
}

Pros:

  • No additional fields in struct
  • Users haven't to change code

Cons:

  • Add little implicit overhead to spawn with additional closure (maybe it costs nothing)

I like 4th and 5th. What about you?

@zonyitoo
Copy link
Contributor

zonyitoo commented Jul 7, 2016

For the 1st, 2nd, 3rd approaches, get_initial should return an Option<usize>, which ensure that you can only get it once, otherwise, when you call get_cross after set_cross, the return result will become confusing.

Hmm, I personally prefer the 4th approach, but it forces user to return an usize value even they don't need to.

@therustmonk
Copy link
Contributor Author

I think we have to follow 4th. Reasons:

  • It's explicit (user have full control)
  • No additional fields in structs
  • If somebody want more Rubish spawn version (without return value and initial data), he can make own which wraps closure with initial and return values (also explicit)
  • User can use different closures builders with different return values (not usize::MAX only)
  • No hidden behaviour

My experience: when I was stumbled upon a segmentation fault, I spend time to find the cause, and explore library's code, but when somebody user who forced to use explicit values will take a bug he'll say: "Ok, I return bad value (pointer) in closure. It's simple to fix.".
Yes, user have to update some code, but with this explicit behaviour some of they can find hidden bugs, I think.

Is there no caveats I'll implement it?

@zonyitoo
Copy link
Contributor

zonyitoo commented Jul 7, 2016

Sure.

@therustmonk
Copy link
Contributor Author

Done! )

@zonyitoo zonyitoo merged commit 9d1ec65 into rustcc:master Jul 7, 2016
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.

2 participants