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

Add as_pin_ref and as_pin_mut to Either #2629

Closed
thomaseizinger opened this issue Jul 27, 2022 · 6 comments · Fixed by #2691
Closed

Add as_pin_ref and as_pin_mut to Either #2629

thomaseizinger opened this issue Jul 27, 2022 · 6 comments · Fixed by #2691

Comments

@thomaseizinger
Copy link
Contributor

Over at either, we are discussing to add structural projection.

Would that be a safe thing to do? What are the reasons why it is not public here in futures::Either?

Any advice is greatly appreciated.

@taiki-e
Copy link
Member

taiki-e commented Jul 27, 2022

Option has as_pin_ref and as_pin_mut for this purpose. It's okay to provide Either::{as_pin_ref, as_pin_mut}, as these are sound unless you write other (wrong) unsafe code.

@thomaseizinger
Copy link
Contributor Author

Thank you for answering so quickly and thoroughly :)

This is resolved from my end unless you want to keep it open to maybe also expose futures::Either::project publicly?

@taiki-e taiki-e reopened this Aug 21, 2022
@taiki-e taiki-e changed the title Would it be safe to expose Either::project publicly? Add as_pin_ref and as_pin_mut to Either Aug 21, 2022
@taiki-e
Copy link
Member

taiki-e commented Aug 21, 2022

Re-opening -- I think it is reasonable to have such methods in futures.

@thomaseizinger
Copy link
Contributor Author

Would it make sense to depend on either instead?

@cuviper
Copy link
Member

cuviper commented Aug 22, 2022

It's possible, but that would be a breaking change to combine them, as one can currently have both impl LocalTrait for either::Either and for futures::future::Either.

@taiki-e
Copy link
Member

taiki-e commented Aug 24, 2022

Would it make sense to depend on either instead?

In addition to it being a breaking change, we are also implementing stream and async io traits on our Either type, so we do not plan to depend on either crate. (e.g., it makes #2362 impossible)

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

Successfully merging a pull request may close this issue.

3 participants