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 a examples to the module docs #411

Merged
merged 1 commit into from
Oct 2, 2017
Merged

Conversation

dns2utf8
Copy link
Contributor

@dns2utf8 dns2utf8 commented Sep 5, 2017

Hi

I wanted to add examples to the docs. My first example works as I expected.

My second example behaves very strange. It compiles when I use i32 as the type of the Range<T>.
With any other type such as usize, u64 or i64 it does not compile.
Do you have an idea how to fix it?

Regards


Error message:

---- src/range.rs - range::Iter (line 23) stdout ----
        error[E0599]: no method named `zip` found for type `rayon::range::Iter<i64>` in the current scope
 --> src/range.rs:6:20
  |
6 |                   .zip(0..25)
  |                    ^^^
  |
  = note: the method `zip` exists but the following trait bounds were not satisfied:
          `&mut rayon::range::Iter<i64> : std::iter::Iterator`

@cuviper
Copy link
Member

cuviper commented Sep 5, 2017

Range<usize> should work, but it's expected that i64 and u64 ranges don't implement IndexedParallelIterator, for the same reason they don't implement std::iter::ExactSizeIterator: a 64-bit range may overflow a usize length on 32-bit platforms.

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Sep 6, 2017

Ah, I did not know that. I added a note to the docs.

@cuviper
Copy link
Member

cuviper commented Sep 16, 2017

I cherry-picked your CI fixes in #436. If you'll please rebase the rest of your changes, we can see how CI fares now.

@dns2utf8
Copy link
Contributor Author

Rebased and tested. Thank you for the cerry-picking.

src/range.rs Outdated
//!
//! let r = (0..100u64).into_par_iter()
//! .fold(|| 0, |acc, cur| acc + cur)
//! .sum();
Copy link
Member

Choose a reason for hiding this comment

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

I notice in both examples you use .fold(...).sum(), when just .sum() should work fine. Was this intentional?

src/range.rs Outdated

use iter::*;
use iter::internal::*;
use std::ops::Range;

/// Parallel iterator over a range
/// Implemented for `u32`, `i32`, `usize` and `isize`.
Copy link
Member

Choose a reason for hiding this comment

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

We should have a blank doc-line between the intro and additional description. Or if you want it as one intro paragraph, then it needs punctuation between the clauses.

Also, ParallelIterator is implemented for range::Iter<T> of all integer types. The difference is just that i64 and u64 don't implement IndexedParallelIterator, which you'd need for zip, for example.

@dns2utf8
Copy link
Contributor Author

Hm the fold().sum() was a strange idea.

@dns2utf8
Copy link
Contributor Author

Would you like me to squash the commits?

src/iter/mod.rs Outdated
@@ -781,6 +781,8 @@ impl<T: ParallelIterator> IntoParallelIterator for T {
/// An iterator that supports "random access" to its data, meaning
/// that you can split it at arbitrary indices and draw data from
/// those points.
///
/// **Note:** Not implemented for `u64` and `i64`
Copy link
Member

Choose a reason for hiding this comment

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

Small clarification, please: "Not implemented for u64 and i64 ranges."

@cuviper
Copy link
Member

cuviper commented Sep 26, 2017

Yeah, let's squash this one to its final state, thanks!

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Oct 2, 2017

Fixed and squashed. Greetings from the RustFest impl days

@cuviper cuviper merged commit e8c0212 into rayon-rs:master Oct 2, 2017
@cuviper
Copy link
Member

cuviper commented Oct 2, 2017

Thanks!

@dns2utf8 dns2utf8 deleted the doc_ranges branch October 3, 2017 08:06
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