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

Get rid of Chain as the building block for other combinators and remo… #2117

Closed
wants to merge 1 commit into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Apr 6, 2020

…ve some unsafe code

This is just the first step in a potential refactor that could remove a lot of safe and unsafe code, and I wanted to test the waters before going all out. As you can see the amount of code that can be deleted this way is high!

The basic ideas are:

  • Switch from pin-utils to pin-project as it has a safe API and supports enums, which results in smaller combinator sizes in memory.
  • Remove Chain as it is unnecessarily complicated. We can instead build up from primitive combinators like Flatten and Map.

There are a couple of open questions:

  1. Is it OK to use raw type aliases as I have done with Then? Strictly speaking, it's a breaking change, however, it's exceptionally unlikely that it will actually cause any problems. If we don't want to do this, the fallback would be to have a macro to define a load of newtype wrappers that automatically implement the relevant traits.

  2. Is it OK to get rid of the unused type parameter on Then? This is a breaking change, although you wouldn't typically name this parameter during normal usage. If we don't want to get rid of it, then we'll need a newtype wrapper for at least Then and any other combinators with unused generic parameters.

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 6, 2020

Oh, and this also fixes #2118 :)

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 6, 2020

The test select_on_non_unpin_size is failing because I reduced the size of the future combinator, but I have no idea what it's actually trying to test by asserting on that size? Am I safe to just update the size?

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
Poll::Ready(loop {
match self.as_mut().project() {
__InternalFlattenProjection::First(f) => {
Copy link
Member

@Nemo157 Nemo157 Apr 7, 2020

Choose a reason for hiding this comment

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

This is an internal type of the generated code which shouldn't be used. You can use the #[project] attribute to allow matching on the projected variants (see https://docs.rs/pin-project/0.4.8/pin_project/attr.pin_project.html#enums)

Copy link
Contributor Author

@Diggsey Diggsey Apr 7, 2020

Choose a reason for hiding this comment

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

Yeah, I've fixed that in the larger PR I have :)

@Nemo157
Copy link
Member

Nemo157 commented Apr 7, 2020

The test select_on_non_unpin_size is failing because I reduced the size of the future combinator, but I have no idea what it's actually trying to test by asserting on that size? Am I safe to just update the size?

That is failing because of changes to the generator transform in rustc, I've opened #2119 to fix it.

@Diggsey
Copy link
Contributor Author

Diggsey commented May 15, 2020

Superceded by #2128

@Diggsey Diggsey closed this May 15, 2020
@Diggsey Diggsey deleted the snippety-snip branch May 15, 2020 17:40
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