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

Fix slicing of views of IndexedSeqs (including fixing 2.13.7-only reverseIterator regression) #9799

Merged
merged 4 commits into from Nov 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 17 additions & 15 deletions src/library/scala/collection/IndexedSeqView.scala
Expand Up @@ -84,8 +84,8 @@ object IndexedSeqView {
}
@SerialVersionUID(3L)
private[collection] class IndexedSeqViewReverseIterator[A](self: IndexedSeqView[A]) extends AbstractIterator[A] with Serializable {
private[this] var pos = self.length - 1
private[this] var remainder = self.length
private[this] var pos = remainder - 1
@inline private[this] def _hasNext: Boolean = remainder > 0
def hasNext: Boolean = _hasNext
def next(): A =
Expand All @@ -96,23 +96,25 @@ object IndexedSeqView {
r
} else Iterator.empty.next()

override def drop(n: Int): Iterator[A] = {
if (n > 0) {
pos -= n
remainder = Math.max(0, remainder - n)
// from < 0 means don't move pos, until < 0 means don't limit remainder
//
override protected def sliceIterator(from: Int, until: Int): Iterator[A] = {
if (_hasNext) {
if (remainder <= from) remainder = 0 // exhausted by big skip
else if (from <= 0) { // no skip, pos is same
if (until >= 0 && until < remainder) remainder = until // ...limited by until
}
else {
pos -= from // skip ahead
if (until >= 0 && until < remainder) { // ...limited by until
if (until <= from) remainder = 0 // ...exhausted if limit is smaller than skip
else remainder = until - from // ...limited by until, less the skip
}
else remainder -= from // ...otherwise just less the skip
}
}
this
}

override def sliceIterator(from: Int, until: Int): Iterator[A] = {
val startCutoff = pos
som-snytt marked this conversation as resolved.
Show resolved Hide resolved
val untilCutoff = startCutoff - remainder + 1
val nextStartCutoff = if (from < 0) startCutoff else if (startCutoff - from < 0) 0 else startCutoff - from
val nextUntilCutoff = if (until < 0) startCutoff else if (startCutoff - until < untilCutoff) untilCutoff else startCutoff - until + 1
remainder = Math.max(0, nextStartCutoff - nextUntilCutoff + 1)
pos = nextStartCutoff
this
}
}

/** An `IndexedSeqOps` whose collection type and collection type constructor are unknown */
Expand Down
19 changes: 8 additions & 11 deletions src/library/scala/collection/Iterator.scala
Expand Up @@ -409,9 +409,9 @@ trait Iterator[+A] extends IterableOnce[A] with IterableOnceOps[A, Iterator, Ite

def indexWhere(p: A => Boolean, from: Int = 0): Int = {
var i = math.max(from, 0)
drop(from)
while (hasNext) {
if (p(next())) return i
val dropped = drop(from)
while (dropped.hasNext) {
if (p(dropped.next())) return i
i += 1
}
-1
Expand Down Expand Up @@ -635,14 +635,7 @@ trait Iterator[+A] extends IterableOnce[A] with IterableOnceOps[A, Iterator, Ite
def next() = if (hasNext) { hdDefined = false; hd } else Iterator.empty.next()
}

def drop(n: Int): Iterator[A] = {
var i = 0
while (i < n && hasNext) {
next()
i += 1
}
this
}
def drop(n: Int): Iterator[A] = sliceIterator(n, -1)
som-snytt marked this conversation as resolved.
Show resolved Hide resolved
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

def dropWhile(p: A => Boolean): Iterator[A] = new AbstractIterator[A] {
// Magic value: -1 = hasn't dropped, 0 = found first, 1 = defer to parent iterator
Expand Down Expand Up @@ -972,6 +965,7 @@ object Iterator extends IterableFactory[Iterator] {
def hasNext = false
def next() = throw new NoSuchElementException("next on empty iterator")
override def knownSize: Int = 0
override protected def sliceIterator(from: Int, until: Int) = this
som-snytt marked this conversation as resolved.
Show resolved Hide resolved
}

/** Creates a target $coll from an existing source collection
Expand All @@ -989,6 +983,9 @@ object Iterator extends IterableFactory[Iterator] {
private[this] var consumed: Boolean = false
def hasNext = !consumed
def next() = if (consumed) empty.next() else { consumed = true; a }
override protected def sliceIterator(from: Int, until: Int) =
if (consumed || from > 0 || until == 0) empty
else this
}

override def apply[A](xs: A*): Iterator[A] = xs.iterator
Expand Down
5 changes: 5 additions & 0 deletions test/junit/scala/collection/IndexedSeqViewTest.scala
Expand Up @@ -18,4 +18,9 @@ class IndexedSeqViewTest {
assertEquals(2, IndexedSeq(1, 2, 3, 4, 5).view.iterator.take(2).knownSize)
assertEquals(2, IndexedSeq(1, 2, 3, 4, 5).view.iterator.slice(2, 4).knownSize)
}

@Test
def reverseEmptyIterator(): Unit = {
assertEquals(0, Vector.empty[Int].reverseIterator.take(1).toList.size)
}
}
14 changes: 7 additions & 7 deletions test/junit/scala/collection/immutable/SmallMapTest.scala
Expand Up @@ -6,14 +6,14 @@ import org.junit._
import scala.tools.testkit.AllocationTest

class SmallMapTest extends AllocationTest {
def iterator(m:Map[_,_]) = m.iterator
def keysIterator(m:Map[_,_]) = m.keysIterator
def valuesIterator(m:Map[_,_]) = m.valuesIterator
def iterator(m: Map[_,_]) = m.iterator
def keysIterator(m: Map[_,_]) = m.keysIterator
def valuesIterator(m: Map[_,_]) = m.valuesIterator

//we use this side effect to avoid the git optimising away the tuples
//but without affecting the allocations
// we use this side effect to avoid the jit optimising away the tuples
// but without affecting the allocations
val nonAllocationResult = new Array[Any](5)
def consume(it:Iterator[_]): Int = {
def consume(it: Iterator[_]): Int = {
var size = 0
nonAllocationResult(0) = it
while (it.hasNext) {
Expand All @@ -22,7 +22,7 @@ class SmallMapTest extends AllocationTest {
}
size
}
def consume1(it:Iterator[_]): Int = {
def consume1(it: Iterator[_]): Int = {
nonAllocationResult(0) = it
nonAllocationResult(1) = it.next()
1
Expand Down