Address reviewers comments

Author: odersky

src/main/scala/strawman/collection/Iterable.scala

@@ -54,7 +54,7 @@ trait IterableOps[+A] extends Any {
   protected def coll: Iterable[A]
   private def iterator() = coll.iterator()
 
-  /** Apply `f` to each element for tis side effects
+  /** Apply `f` to each element for its side effects
    *  Note: [U] parameter needed to help scalac's type inference.
    */
   def foreach[U](f: A => U): Unit = iterator().foreach(f)

src/main/scala/strawman/collection/mutable/ArrayBuffer.scala

@@ -28,6 +28,11 @@ class ArrayBuffer[A] private (initElems: Array[AnyRef], initLength: Int)
     end = n
   }
 
+  private def checkWithinBounds(lo: Int, hi: Int) = {
+    if (lo < 0) throw new IndexOutOfBoundsException(lo.toString)
+    if (hi > end) throw new IndexOutOfBoundsException(hi.toString)
+  }
+
   def apply(n: Int) = array(n).asInstanceOf[A]
 
   def update(n: Int, elem: A): Unit = array(n) = elem.asInstanceOf[AnyRef]
@@ -69,14 +74,14 @@ class ArrayBuffer[A] private (initElems: Array[AnyRef], initLength: Int)
   def result = this
 
   def insert(idx: Int, elem: A): Unit = {
-    if (idx < 0 || idx > end) throw new IndexOutOfBoundsException
+    checkWithinBounds(idx, idx)
     ensureSize(end + 1)
     Array.copy(array, idx, array, idx + 1, end - idx)
     this(idx) = elem
   }
 
   def insertAll(idx: Int, elems: IterableOnce[A]): Unit = {
-    if (idx < 0 || idx > end) throw new IndexOutOfBoundsException
+    checkWithinBounds(idx, idx)
     elems match {
       case elems: collection.Iterable[A] =>
         val elemsLength = elems.size
@@ -100,7 +105,7 @@ class ArrayBuffer[A] private (initElems: Array[AnyRef], initLength: Int)
   }
 
   def remove(idx: Int): A = {
-    if (idx < 0 || idx >= end) throw new IndexOutOfBoundsException
+    checkWithinBounds(idx, idx + 1)
     val res = this(idx)
     Array.copy(array, idx + 1, array, idx, end - (idx + 1))
     reduceToSize(end - 1)
@@ -109,7 +114,7 @@ class ArrayBuffer[A] private (initElems: Array[AnyRef], initLength: Int)
 
   def remove(from: Int, n: Int): Unit =
     if (n > 0) {
-      if (from < 0 || from + n > end) throw new IndexOutOfBoundsException
+      checkWithinBounds(from, from + n)
       Array.copy(array, from + n, array, from, end - (from + n))
       reduceToSize(end - n)
     }
@@ -158,6 +163,7 @@ object RefArrayUtils {
   /** Remove elements of this array at indices after `sz`.
    */
   def nullElems(array: Array[AnyRef], start: Int, end: Int): Unit = {
+    // Maybe use `fill` instead?
     var i = start
     while (i < end) {
       array(i) = null

src/main/scala/strawman/collection/mutable/Growable.scala

@@ -1,11 +1,3 @@
-/*                     __                                               *\
-**     ________ ___   / /  ___     Scala API                            **
-**    / __/ __// _ | / /  / _ |    (c) 2003-2013, LAMP/EPFL             **
-**  __\ \/ /__/ __ |/ /__/ __ |    http://scala-lang.org/               **
-** /____/\___/_/ |_/____/_/ | |                                         **
-**                          |/                                          **
-\*                                                                      */
-
 package strawman.collection.mutable
 
 import strawman.collection.IterableOnce
@@ -16,14 +8,6 @@ import strawman.collection.{toOldSeq, toNewSeq}
 /** This trait forms part of collections that can be augmented
  *  using a `+=` operator and that can be cleared of all elements using
  *  a `clear` method.
- *
- *  @author   Martin Odersky
- *  @version 2.8
- *  @since   2.8
- *  @define coll growable collection
- *  @define Coll `Growable`
- *  @define add  add
- *  @define Add  add
  */
 trait Growable[-A] {
 
@@ -59,6 +43,8 @@ trait Growable[-A] {
       case xs: scala.collection.LinearSeq[_] => loop(xs.asInstanceOf[scala.collection.LinearSeq[A]])
       case xs => xs.iterator() foreach += // Deviation: IterableOnce does not define `foreach`.
     }
+    // @ichoran writes: Right now, this actually isn't any faster than using an iterator
+    // for List. Maybe we should just simplify the code by deferring to iterator foreach?
     this
   }
 

src/main/scala/strawman/collection/mutable/ListBuffer.scala

@@ -66,7 +66,6 @@ class ListBuffer[A]
     if (i == 0) null
     else if (i == len) last
     else {
-      assert(i > 0 && i < len)
       var j = i - 1
       var p = first
       while (j > 0) {
@@ -188,7 +187,7 @@ class ListBuffer[A]
   def patchInPlace(from: Int, patch: collection.Seq[A], replaced: Int): this.type = {
     ensureUnaliased()
     val p = locate(from)
-    removeAfter(p, replaced `min` length - from)
+    removeAfter(p, replaced `min` (length - from))
     insertAfter(p, patch.iterator())
     this
   }

src/main/scala/strawman/collection/mutable/Seq.scala

@@ -1,6 +1,5 @@
 package strawman.collection.mutable
 
-import java.lang.IndexOutOfBoundsException
 import scala.{Int, Long, Unit, Boolean, Array}
 import strawman.collection
 import strawman.collection.{IterableOnce, toNewSeq, toOldSeq}
@@ -25,9 +24,11 @@ trait GrowableSeq[A] extends Seq[A]
   def patchInPlace(from: Int, patch: collection.Seq[A], replaced: Int): this.type
 
   // +=, ++=, clear inherited from Growable
-  def +=:(elem: A): this.type = { insert(0, elem); this }
-  def +=:(elem1: A, elem2: A, elems: A*): this.type = elem1 +=: elem2 +=: elems.toStrawman ++=: this
-  def ++=:(elems: IterableOnce[A]): this.type = { insertAll(0, elems); this }
+  // Per remark of @ichoran, we should preferably not have these:
+  //
+  // def +=:(elem: A): this.type = { insert(0, elem); this }
+  // def +=:(elem1: A, elem2: A, elems: A*): this.type = elem1 +=: elem2 +=: elems.toStrawman ++=: this
+  // def ++=:(elems: IterableOnce[A]): this.type = { insertAll(0, elems); this }
 
   def dropInPlace(n: Int): this.type = { remove(0, n); this }
   def dropRightInPlace(n: Int): this.type = { remove(length - n, n); this }
@@ -52,13 +53,15 @@ trait GrowableSeq[A] extends Seq[A]
 trait IndexedOptimizedSeq[A] extends Seq[A] {
   def mapInPlace(f: A => A): this.type = {
     var i = 0
-    while (i < size) { this(i) = f(this(i)); i += 1 }
+    val siz = size
+    while (i < siz) { this(i) = f(this(i)); i += 1 }
     this
   }
 }
 
 trait IndexedOptimizedGrowableSeq[A] extends IndexedOptimizedSeq[A] with GrowableSeq[A] {
   def flatMapInPlace(f: A => IterableOnce[A]): this.type = {
+    // There's scope for a better implementation which copies elements in place.
     var i = 0
     val newElemss = new Array[IterableOnce[A]](size)
     while (i < size) { newElemss(i) = f(this(i)); i += 1 }
@@ -69,7 +72,8 @@ trait IndexedOptimizedGrowableSeq[A] extends IndexedOptimizedSeq[A] with Growabl
   }
   def filterInPlace(p: A => Boolean): this.type = {
     var i = 0
-    var j = 0
+    while (i < size && p(apply(i))) i += 1
+    var j = 1
     while (i < size) {
       if (p(apply(i))) {
         this(j) = this(i)