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 Update adaptor and `update()` method #488

Merged
merged 2 commits into from Dec 8, 2017

Conversation

Projects
None yet
2 participants
@Enet4
Copy link
Contributor

Enet4 commented Dec 8, 2017

This resolves #487. The sequential version created by the producer is based on the itertools implementation. Tests are included, but let me know if you'd like better coverage.

@cuviper
Copy link
Member

cuviper left a comment

This looks great, thanks!

I do want a few more tests just for uniformity, adding this to tests/clones.rs, tests/debug.rs, tests/producer_split_at.rs, and tests/compile-fail/must_use.rs. I expect this will behave fine though. :)

/// ```
/// use rayon::prelude::*;
///
/// let mut par_iter = (0..5).into_par_iter().update(|mut x| {*x *= 2;});

This comment has been minimized.

@cuviper

cuviper Dec 8, 2017

Member

nit: I don't think you need either mut here. par_iter will be moved into collect(), and x is already &mut.

}
}

/// Standard Update adaptor, based on `itertools::adaptors::Update`

This comment has been minimized.

@cuviper

cuviper Dec 8, 2017

Member

Did you consider using itertools directly? We wouldn't want a public dependency, but this is behind the scenes. But the implementation is pretty straightforward, so I'm just wondering what you think.

This comment has been minimized.

@Enet4

Enet4 Dec 8, 2017

Author Contributor

I did think about that while reading the issue. I chose to replicate the implementation because interleave did the same, although with additional reasons.

If there is willingness to depend on itertools in future versions, I suppose this adaptor would take advantage of that. Although it's just my opinion, leaving these adaptors behind a feature gate to keep itertools an optional dependency feels a bit unnecessary.

///
/// assert_eq!(&doubles[..], &[0, 2, 4, 6, 8]);
/// ```
fn update<F>(self, update_op: F) -> Update<Self, F>

This comment has been minimized.

@cuviper

cuviper Dec 8, 2017

Member

Also, just to keep related behavior grouped together, let's move this up after inspect.

Adjust Update adaptor.
- Add set of adaptor tests
- Move `update` method
- Remove extraneous `mut`s

@Enet4 Enet4 force-pushed the Enet4:update_adaptor branch from 40ef3a8 to 0031f91 Dec 8, 2017

@Enet4

This comment has been minimized.

Copy link
Contributor Author

Enet4 commented Dec 8, 2017

The requested adjustments were made. Please let me know if you'd like a commit squash.

As for the actual use of itertools, if you find that keeping it as a dependency is ok, I can make the necessary changes.

@cuviper

cuviper approved these changes Dec 8, 2017

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Dec 8, 2017

Thanks! We don't need to worry much about itertools -- we can always change this later if wanted.

bors r+

bors bot added a commit that referenced this pull request Dec 8, 2017

Merge #488
488: Add Update adaptor and `update()` method r=cuviper a=Enet4

This resolves #487. The sequential version created by the producer is based on the `itertools` implementation. Tests are included, but let me know if you'd like better coverage.
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Dec 8, 2017

@bors bors bot merged commit 0031f91 into rayon-rs:master Dec 8, 2017

2 checks passed

bors Build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Enet4 Enet4 deleted the Enet4:update_adaptor branch Dec 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.