Add Repeat

[ChristopherDavenport]

Creates a Parallel Iterator that endlessly repeats a single element

I am sure this needs more testing, however I don't know the best way. I believe I have a working implementation.

Refs #331

[nikomatsakis]

Nit: I'd format this as Repeat { element: elt }

[cuviper]

I'd suggest just running rustfmt repeat.rs to probably address all of the nits at once.

[nikomatsakis]

Nit: same here

[nikomatsakis]

Nit: indentation

[nikomatsakis]

Nit: the { should align with fn

[nikomatsakis]

Nit: CB::Output

[nikomatsakis]

Nit: only one of these needs to be a clone

[nikomatsakis]

Nit: only one of these needs to be a clone; the other can just be repeat: self.repeat and take ownership

[nikomatsakis]

Nit: Self::IntoIter {

[nikomatsakis]

Argh. The need to have ExactSizeIterator implemented is frustrating; I'm not sure if this impl is technically obeying the intentions of this trait (although I guess it works fine in practice). I'm trying to remember why we need ExactSizeIterator; something to do with the reverse operation iirc.

[cuviper]

Because std::iter::Zip is only double-ended if its inputs are double-ended and exact-size.

[nikomatsakis]

Yeah, I remembered it had something to do with Zip

[cuviper]

Since your producer's split_at doesn't actually use the index, I don't think this take(4) actually works -- but the result is still limited by the zip length instead. Try another test with just repeat(4).take(4).collect(), and it panics "too many values pushed to consumer".

[ChristopherDavenport]

Is there something I should do in order to get take to work effectively with this implementation. It seems to play nicely with others, but no so nicely by itself.

[cuviper]

The producer will need to track its effective length. It should initialize to usize::MAX since that's what len() reported. Then each time it splits, use the given index to determine the resulting length of each side. And finally the iterator will also need to limit its length.

[ChristopherDavenport]

So basically despite this being infinite in this formulation, when managing the IndexedParralellIterator actually limit the size to usize::MAX and reduce as we consume. So we can split where necessary.

[cuviper]

Yes, exactly. We can't use this as an IndexedParallelIterator unless we actually map it to indexes, which puts an upper bound on the length.

Well... actually if you want to be clever, you could track the length as Option<usize> and leave the rightmost part of the splits open-ended. So a bounded segment with Some(len) splits into two bounded segments, and an unbounded None splits into Some on the left and None on the right. This will still work with take because it splits and discards the (infinite) remainder.

[cuviper]

This should use bridge_unindexed instead, otherwise you're never using the UnindexedProducer implementation at all.

[cuviper]

I wonder, why does this store the whole Repeat type, and not just the element? It's a minor difference, but then we don't need to worry about Clone for Repeat itself.

[cuviper]

Same here, seems like this could just store the element T.

[ChristopherDavenport]

I have not forgotten about this, sorry about the long delay. Hopefully can get around to it by tomorrow evening or at latest this weekend.

[nikomatsakis]

Can we add some docs to this?

[nikomatsakis]

I think this should type should require T: Clone + Send

[nikomatsakis]

Also, it should probably have some documentation, since it is user-facing.

[nikomatsakis]

Nit: Can we make this <T> instead of <P>, so it matches the others? This confused me for a while, since I thought P must imply it is a producer or something. I'd also rather see T: Clone + Send.

[nikomatsakis]

I'm still mildly nervous about this but I don't have a better suggestion. =) I feel like maybe we should revisit the way we handle rev() to see if we can lift this requirement; but I guess it's ok for now.

[nikomatsakis]

This is looking great to me! I made a few minor suggestions, mostly about rustdocs and conventions -- do you think you'll have time to apply those?

[nikomatsakis]

This is looking great to me! I made a few minor suggestions, mostly about rustdocs and conventions -- do you think you'll have time to apply those?

[nikomatsakis]

@ChristopherDavenport do you still have time to make further changes here? (If not, that's fine, just want to know -- I can try to make the final tweaks myself.)

[ChristopherDavenport]

I may need you to finish the final tweaks.

I will not bore you with in depth details(sisters graduation, switching positions, 2 emergency applications needed asap for work). My experience with rust being so weak I haven't been able to yank myself out of my work, primary OSS long enough to finish this.

I apologize and if you don't finish it up I will fix it myself eventually, but the "eventually" is a problem I can't nail down at the moment.

[nikomatsakis]

@ChristopherDavenport no worries at all! I very much appreciate the work you did so far.

[cuviper]

We can't just ignore lengths in the indexed iterator. See my comment on split_at.

One thing we discussed on gitter, which might be more palatable than lying about lengths, is to stop trying to make Repeat an indexed iterator. Let it be unindexed only, and add a second variant like repeatn<T>(elt: T, n: usize) -> RepeatN<T> that is indexed, conceptually equivalent to repeat(elt).take(n).

Niko thought we could even add an inherent method Repeat::take(usize) -> RepeatN to let repeat(elt).take(n) still work, but we probably don't want to duplicate too many indexed methods that way.

[cuviper]

We can't just ignore lengths in the indexed iterator. See my comment on split_at.

One thing we discussed on gitter, which might be more palatable than lying about lengths, is to stop trying to make Repeat an indexed iterator. Let it be unindexed only, and add a second variant like repeatn<T>(elt: T, n: usize) -> RepeatN<T> that is indexed, conceptually equivalent to repeat(elt).take(n).

Niko thought we could even add an inherent method Repeat::take(usize) -> RepeatN to let repeat(elt).take(n) still work, but we probably don't want to duplicate too many indexed methods that way.

[cuviper]

I think this is still wrong. If I repeat(x).take(4), then we'll get a split_at(4), and this needs to remember to only produce 4 items on the left side. The right side is debatable, whether we leave that as infinite/usize::MAX, or now reduce it to MAX - 4 remaining.

(noted here before: #337 (comment))

[nikomatsakis]

Ah yes, good point. On this note, I'm softening on repeatn. Seems like a useful building block.

[nikomatsakis]

Maybe we should just not offer repeat at all; you can use repeatn(usize::MAX) for all practical purposes.

[nikomatsakis]

Closing in favor of #397

[nikomatsakis]

Thanks very much @ChristopherDavenport for laying the foundation here!