Remove dropRight and takeRight from Iterator #150
Conversation
| protected val normN = n max 0 | ||
| override def knownSize = | ||
| if (underlying.knownSize >= 0) (underlying.knownSize - normN) max 0 else -1 | ||
| } |
There was a problem hiding this comment.
I removed the DropRight view which was based on the dropRight method of Iterator. (same for TakeRight below)
| } | ||
| } | ||
|
|
||
| override def knownSize: Int = length |
There was a problem hiding this comment.
That’s unrelated to the PR, actually, but I thought we could override knownSize in the case of IndexedView. We should do the same for IndexedSeq.
| private val delta = underlying.length - normN | ||
| def length = underlying.length min normN | ||
| def apply(i: Int) = underlying.apply(i) | ||
| def apply(i: Int) = underlying.apply(i + delta) |
There was a problem hiding this comment.
I kept the IndexedView based implementation of takeRight (and dropRight, below)
| val lead = coll.iterator() drop n | ||
| val it = coll.iterator() | ||
| while (lead.hasNext) { | ||
| lead.next() |
There was a problem hiding this comment.
This is a cute algorithm, but I can't imagine it's faster than just going
var m = this.size
val it = coll.iterator()
while (m > n) it.next
while (it.hasNext) b += it.next()
There was a problem hiding this comment.
That would have the benefit of requiring only one traversal, indeed (excepted in the case of linear collections). I should give it a try.
There was a problem hiding this comment.
I tried it but didn’t notice a difference (in List):
There was a problem hiding this comment.
Wait, what? List shouldn't rebuild on takeRight anyway--if it does, that needs to be fixed. Also, I didn't suggest the other method is faster, just more obviously correct.
There was a problem hiding this comment.
Indeed good point, that’s anyway something that we should fix… (#162 keeps track of that)
| def dropRight(n: Int): C = fromSpecificIterable(View.DropRight(coll, n)) | ||
| def dropRight(n: Int): C = { | ||
| val b = newSpecificBuilder() | ||
| if (n >= 0) b.sizeHint(coll, -n) |
There was a problem hiding this comment.
This version of sizeHint takes a collection and a delta to apply to the collection’s size. Here we want to say that the size of the result will be the size of coll minus the number of dropped elemnts. I could write explicitly sizeHint(coll, delta = -n), for clarity.
| extends View.TakeRight(underlying, n) with IndexedView[A] { | ||
| override def iterator() = super.iterator() // needed to avoid "conflicting overrides" error | ||
| class TakeRight[A](underlying: IndexedView[A], n: Int) extends IndexedView[A] { | ||
| private val normN = n max 0 |
There was a problem hiding this comment.
You don't need to store this value. def length = underlying.length - delta is enough.
| extends View.DropRight(underlying, n) with IndexedView[A] { | ||
| override def iterator() = super.iterator() | ||
| class DropRight[A](underlying: IndexedView[A], n: Int) extends IndexedView[A] { | ||
| private val normN = n max 0 |
There was a problem hiding this comment.
You can just store the actual length here; you never need n max 0 separately.
|
Looks reasonable. I made some comments but didn't do a formal review. |
2 participants
Fixes #131.
Note that I kept the
groupedandslidingmethods ofIteratoreven though they also use buffers. Since these methods do exist in 2.12 we have to discuss before we remove them…