AndWass / pushgen

Apache License 2.0
27 stars 4 forks source link

[Question] pushgen::Generator and std::ops::Generator #32

Closed NobodyXu closed 3 years ago

NobodyXu commented 3 years ago

The name of pushgen::Generator reminds me of the std::ops::Generator which has similar name and similar purpose.

It seems that std::ops::Generator actually maps quite well to pushgen::Generator:

pub trait Generator<R = ()> {
    type Yield;
    type Return;
    fn resume(self: Pin<&mut Self>, resume: R) -> GeneratorState<Self::Yield, Self::Return>;
}

One can map R to the function parameter in pushgen::Generator and Yield, Return can be interpreted as true and false.

AndWass commented 3 years ago

Yes that seems to be the case.

Though std::ops::Generator is an unstable API and more generic than pushgen::Generator. The Pin argument also makes for a clunkier API in my opinion.

NobodyXu commented 3 years ago

Pin argument definitly makes it less of an ideal API, I am not sure why they require that as you cannot have two mutable reference to one variable.

Other than that, do you think it will have any use in pushgen once it is stablized?

For example, would it make the implementation of pushgen easier?

AndWass commented 3 years ago

Looking at the documentation and at the tracking issue: https://github.com/rust-lang/rust/issues/43122 it looks to be tied to coroutines. I don't think it will make it easier to implement pushgen to be honest. pushgen generators are more closely related to iterators rather than coroutine generators.

NobodyXu commented 3 years ago

It is indeed tied to coroutines, though according to generators - unstable book it is actually compiled into a stack machine underlying, which IMHO can be used to simplify implementation of Dedup by removing the need to use Option as a poor man’s solution for state machine, removing the need to unwrap that option even if it is never None inside the loop while still readable..

AndWass commented 3 years ago

Yah for that reason it could maybe be used as an implementation-detail. Benchmarks would have to be done though!

Regarding the unwrap() I am waiting for unwrap_unchecked to be stabilized exactly for Dedup (will probably try it by-hand as well)

AndWass commented 3 years ago

But the Pin is IMO sort of a blocker for public API

NobodyXu commented 3 years ago

Yah for that reason it could maybe be used as an implementation-detail. Benchmarks would have to be done though!

Regarding the unwrap() I am waiting for unwrap_unchecked to be stabilized exactly for Dedup (will probably try it by-hand as well)

Maybe it can be implemented manually using:

// unlike unreachable!() which will panic on reaching, unreachable_unchecked
// has no effect on the generated code and it will be UB if it is actually reached.
use std::hint::unreachable_unchecked;

fn unwrap_unchecked<T>(option: Option<T>) -> T {
    match option {
        Some(val) => val,
        None => unsafe { unreachable_unchecked() }
    }
}
NobodyXu commented 3 years ago

But the Pin is IMO sort of a blocker for public API

That is kindof sad though, since std::ops::Generator almost looks like something designed for pushgen but is blocked by some minor issues.

Another issue with it is that pushgen::Generator might be ran with different function types while the std::ops::Generator probably won’t support it.

To work around it, either the trait has to be implemented manually, which IMHO lost all benefits or

Edit:

It seems that I didn’t take into account that the some callbacks needs to modifiy a count, which only can be done via the second approach.

However, according to my memory, it seems that this only occuies in skip and take which would only pass one type of function anyway.

AndWass commented 3 years ago

Thanks! Yah it's on my to-do to see if that has any measurable impact

NobodyXu commented 3 years ago

I can prepare a PR for the unwrap_checked tomorrow.

My take is that the guarantees that is never panic will be favorable by that projects that have strict requirements on panic and it would save some auto generated unwind code from clang.

AndWass commented 3 years ago

That is kindof sad though, since std::ops::Generator almost looks like something designed for pushgen but is blocked by some minor issues.

Another issue with it is that pushgen::Generator might be ran with different function types while the std::ops::Generator probably won’t support it.

To work around it, either the trait has to be implemented manually, which IMHO lost all benefits or

  • maybe the pushgen::Generator can be modified to accept a function pointer instead and allows the function to also return a value of type Output when on GeneratorResult::Stop, though I am not sure whether this is generic enough
  • or just pass around a &dyn Fn(Item) -> Result if the above approach is not adequate.

True that! And introducing dynamic dispatch (via function pointer or dyn FnMut) is more likely to impact performance more than I am comfortable with.

I like your thinking though!

AndWass commented 3 years ago

I can prepare a PR for the unwrap_checked tomorrow.

My take is that the guarantees that is never panic will be favorable by that projects that have strict requirements on panic and it would save some auto generated unwind code from clang.

That would be much appreciated!

AndWass commented 3 years ago

Can this issue be closed?

NobodyXu commented 3 years ago

Yep