Skip to content

Commit

Permalink
Fix unreported bug in View.Patched
Browse files Browse the repository at this point in the history
Fix unreported bug in `View.Patched` where iterator
is incorrect due to the patch only being iterable once,
and already having been exhausted.

Simplify `ArrayBuffer#insertAll` code.
  • Loading branch information
NthPortal committed Sep 2, 2021
1 parent 75a3f19 commit 20ea0c0
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 22 deletions.
10 changes: 8 additions & 2 deletions src/library/scala/collection/View.scala
Original file line number Diff line number Diff line change
Expand Up @@ -404,9 +404,15 @@ object View extends IterableFactory[View] {

@SerialVersionUID(3L)
private[collection] class Patched[A](underlying: SomeIterableOps[A], from: Int, other: IterableOnce[A], replaced: Int) extends AbstractView[A] {
def iterator: Iterator[A] = underlying.iterator.patch(from, other.iterator, replaced)
// we may be unable to traverse `other` more than once, so we need to cache it if that's the case
private val _other: Iterable[A] = other match {
case other: Iterable[A] => other
case other => LazyList.from(other)
}

def iterator: Iterator[A] = underlying.iterator.patch(from, _other.iterator, replaced)
override def knownSize: Int = if (underlying.knownSize == 0 && other.knownSize == 0) 0 else super.knownSize
override def isEmpty: Boolean = if (knownSize == 0) true else iterator.isEmpty
override def isEmpty: Boolean = if (knownSize == 0) true else _other.isEmpty
}

@SerialVersionUID(3L)
Expand Down
34 changes: 14 additions & 20 deletions src/library/scala/collection/mutable/ArrayBuffer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -177,27 +177,21 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
case elems: collection.Iterable[A] =>
val elemsLength = elems.size
if (elemsLength > 0) {
ensureSize(length + elemsLength)
Array.copy(array, index, array, index + elemsLength, size0 - index)
size0 = size0 + elemsLength
elems match {
case elems: ArrayBuffer[_] =>
// if `elems eq this`, this works because `elems.array eq this.array`,
// we didn't overwrite the values being inserted after moving them in
// the previous copy a few lines up, and `System.arraycopy` will
// effectively "read" all the values before overwriting any of them.
Array.copy(elems.array, 0, array, index, elemsLength)
case _ =>
var i = 0
val it = elems.iterator
while (i < elemsLength) {
this(index + i) = it.next()
i += 1
}
}
val len = size0
val newSize = len + elemsLength
ensureSize(newSize)
Array.copy(array, index, array, index + elemsLength, len - index)
// if `elems eq this`, this copy is safe because
// - `elems.array eq this.array`
// - we didn't overwrite the values being inserted after moving them in
// the previous line
// - `copyElemsToArray` will call `System.arraycopy`
// - `System.arraycopy` will effectively "read" all the values before
// overwriting any of them when two arrays are the the same reference
IterableOnce.copyElemsToArray(elems, array.asInstanceOf[Array[Any]], index, elemsLength)
size0 = newSize // update size AFTER the copy, in case we're inserting a proxy
}
case _ =>
insertAll(index, ArrayBuffer.from(elems))
case _ => insertAll(index, ArrayBuffer.from(elems))
}
}

Expand Down
7 changes: 7 additions & 0 deletions test/junit/scala/collection/ViewTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,11 @@ class ViewTest {
def _toString(): Unit = {
assertEquals("View(<not computed>)", View(1, 2, 3).toString)
}

// see https://github.com/scala/scala/pull/9388/files#r630681050
@Test
def patch(): Unit = {
val view = List(2).view.patch(1, List(3, 4, 5).iterator, 0)
assertEquals(view.toList, view.toList)
}
}

0 comments on commit 20ea0c0

Please sign in to comment.