Merge pull request scala#1298 from pavelpavlov/SI-5767

SI-5767 fix + protecting public FlatHashMap API

Author: jsuereth

src/compiler/scala/tools/nsc/transform/LambdaLift.scala

@@ -154,7 +154,7 @@ abstract class LambdaLift extends InfoTransform {
     private def markCalled(sym: Symbol, owner: Symbol) {
       debuglog("mark called: " + sym + " of " + sym.owner + " is called by " + owner)
       symSet(called, owner) addEntry sym
-      if (sym.enclClass != owner.enclClass) calledFromInner addEntry sym
+      if (sym.enclClass != owner.enclClass) calledFromInner += sym
     }
 
     /** The traverse function */

src/library/scala/collection/mutable/FlatHashTable.scala

@@ -44,7 +44,7 @@ trait FlatHashTable[A] extends FlatHashTable.HashUtils[A] {
    */
   @transient protected var sizemap: Array[Int] = null
 
-  @transient var seedvalue: Int = tableSizeSeed
+  @transient protected var seedvalue: Int = tableSizeSeed
 
   import HashTable.powerOfTwo
 
@@ -109,7 +109,7 @@ trait FlatHashTable[A] extends FlatHashTable.HashUtils[A] {
   }
 
   /** Finds an entry in the hash table if such an element exists. */
-  def findEntry(elem: A): Option[A] = {
+  protected def findEntry(elem: A): Option[A] = {
     var h = index(elemHashCode(elem))
     var entry = table(h)
     while (null != entry && entry != elem) {
@@ -120,7 +120,7 @@ trait FlatHashTable[A] extends FlatHashTable.HashUtils[A] {
   }
 
   /** Checks whether an element is contained in the hash table. */
-  def containsEntry(elem: A): Boolean = {
+  protected def containsEntry(elem: A): Boolean = {
     var h = index(elemHashCode(elem))
     var entry = table(h)
     while (null != entry && entry != elem) {
@@ -133,7 +133,7 @@ trait FlatHashTable[A] extends FlatHashTable.HashUtils[A] {
   /** Add entry if not yet in table.
    *  @return Returns `true` if a new entry was added, `false` otherwise.
    */
-  def addEntry(elem: A) : Boolean = {
+  protected def addEntry(elem: A) : Boolean = {
     var h = index(elemHashCode(elem))
     var entry = table(h)
     while (null != entry) {
@@ -150,7 +150,7 @@ trait FlatHashTable[A] extends FlatHashTable.HashUtils[A] {
   }
 
   /** Removes an entry from the hash table, returning an option value with the element, or `None` if it didn't exist. */
-  def removeEntry(elem: A) : Option[A] = {
+  protected def removeEntry(elem: A) : Option[A] = {
     if (tableDebug) checkConsistent()
     def precedes(i: Int, j: Int) = {
       val d = table.length >> 1
@@ -185,7 +185,7 @@ trait FlatHashTable[A] extends FlatHashTable.HashUtils[A] {
     None
   }
 
-  def iterator: Iterator[A] = new AbstractIterator[A] {
+  protected def iterator: Iterator[A] = new AbstractIterator[A] {
     private var i = 0
     def hasNext: Boolean = {
       while (i < table.length && (null == table(i))) i += 1

src/library/scala/collection/mutable/HashMap.scala

@@ -49,30 +49,31 @@ extends AbstractMap[A, B]
   type Entry = DefaultEntry[A, B]
 
   override def empty: HashMap[A, B] = HashMap.empty[A, B]
-  override def clear() = clearTable()
+  override def clear() { clearTable() }
   override def size: Int = tableSize
 
   def this() = this(null)
 
   override def par = new ParHashMap[A, B](hashTableContents)
 
   // contains and apply overridden to avoid option allocations.
-  override def contains(key: A) = findEntry(key) != null
+  override def contains(key: A): Boolean = findEntry(key) != null
+
   override def apply(key: A): B = {
     val result = findEntry(key)
-    if (result == null) default(key)
+    if (result eq null) default(key)
     else result.value
   }
 
   def get(key: A): Option[B] = {
     val e = findEntry(key)
-    if (e == null) None
+    if (e eq null) None
     else Some(e.value)
   }
 
   override def put(key: A, value: B): Option[B] = {
-    val e = findEntry(key)
-    if (e == null) { addEntry(new Entry(key, value)); None }
+    val e = findOrAddEntry(key, value)
+    if (e eq null) None
     else { val v = e.value; e.value = value; Some(v) }
   }
 
@@ -85,9 +86,8 @@ extends AbstractMap[A, B]
   }
 
   def += (kv: (A, B)): this.type = {
-    val e = findEntry(kv._1)
-    if (e == null) addEntry(new Entry(kv._1, kv._2))
-    else e.value = kv._2
+    val e = findOrAddEntry(kv._1, kv._2)
+    if (e ne null) e.value = kv._2
     this
   }
 
@@ -127,12 +127,19 @@ extends AbstractMap[A, B]
     if (!isSizeMapDefined) sizeMapInitAndRebuild
   } else sizeMapDisable
 
+  protected def createNewEntry[B1](key: A, value: B1): Entry = {
+    new Entry(key, value.asInstanceOf[B])
+  }
+
   private def writeObject(out: java.io.ObjectOutputStream) {
-    serializeTo(out, _.value)
+    serializeTo(out, { entry =>
+      out.writeObject(entry.key)
+      out.writeObject(entry.value)
+    })
   }
 
   private def readObject(in: java.io.ObjectInputStream) {
-    init[B](in, new Entry(_, _))
+    init(in, createNewEntry(in.readObject().asInstanceOf[A], in.readObject()))
   }
 
 }

src/library/scala/collection/mutable/HashSet.scala

@@ -53,7 +53,7 @@ extends AbstractSet[A]
 
   override def companion: GenericCompanion[HashSet] = HashSet
 
-  override def size = tableSize
+  override def size: Int = tableSize
 
   def contains(elem: A): Boolean = containsEntry(elem)
 
@@ -67,7 +67,9 @@ extends AbstractSet[A]
 
   override def remove(elem: A): Boolean = removeEntry(elem).isDefined
 
-  override def clear() = clearTable()
+  override def clear() { clearTable() }
+
+  override def iterator: Iterator[A] = super[FlatHashTable].iterator
 
   override def foreach[U](f: A =>  U) {
     var i = 0

src/library/scala/collection/mutable/HashTable.scala

@@ -32,6 +32,9 @@ package mutable
  *  @tparam A     type of the elements contained in this hash table.
  */
 trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashUtils[A] {
+  // Replacing Entry type parameter by abstract type member here allows to not expose to public
+  // implementation-specific entry classes such as `DefaultEntry` or `LinkedEntry`.
+  // However, I'm afraid it's too late now for such breaking change.
   import HashTable._
 
   @transient protected var _loadFactor = defaultLoadFactor
@@ -52,7 +55,7 @@ trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashU
    */
   @transient protected var sizemap: Array[Int] = null
 
-  @transient var seedvalue: Int = tableSizeSeed
+  @transient protected var seedvalue: Int = tableSizeSeed
 
   protected def tableSizeSeed = Integer.bitCount(table.length - 1)
 
@@ -75,11 +78,10 @@ trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashU
   }
 
   /**
-   * Initializes the collection from the input stream. `f` will be called for each key/value pair
-   * read from the input stream in the order determined by the stream. This is useful for
-   * structures where iteration order is important (e.g. LinkedHashMap).
+   * Initializes the collection from the input stream. `readEntry` will be called for each
+   * entry to be read from the input stream.
    */
-  private[collection] def init[B](in: java.io.ObjectInputStream, f: (A, B) => Entry) {
+  private[collection] def init(in: java.io.ObjectInputStream, readEntry: => Entry) {
     in.defaultReadObject
 
     _loadFactor = in.readInt()
@@ -100,35 +102,34 @@ trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashU
 
     var index = 0
     while (index < size) {
-      addEntry(f(in.readObject().asInstanceOf[A], in.readObject().asInstanceOf[B]))
+      addEntry(readEntry)
       index += 1
     }
   }
 
   /**
    * Serializes the collection to the output stream by saving the load factor, collection
-   * size, collection keys and collection values. `value` is responsible for providing a value
-   * from an entry.
+   * size and collection entries. `writeEntry` is responsible for writing an entry to the stream.
    *
-   * `foreach` determines the order in which the key/value pairs are saved to the stream. To
+   * `foreachEntry` determines the order in which the key/value pairs are saved to the stream. To
    * deserialize, `init` should be used.
    */
-  private[collection] def serializeTo[B](out: java.io.ObjectOutputStream, value: Entry => B) {
+  private[collection] def serializeTo(out: java.io.ObjectOutputStream, writeEntry: Entry => Unit) {
     out.defaultWriteObject
     out.writeInt(_loadFactor)
     out.writeInt(tableSize)
     out.writeInt(seedvalue)
     out.writeBoolean(isSizeMapDefined)
-    foreachEntry { entry =>
-      out.writeObject(entry.key)
-      out.writeObject(value(entry))
-    }
+
+    foreachEntry(writeEntry)
   }
 
   /** Find entry with given key in table, null if not found.
    */
-  protected def findEntry(key: A): Entry = {
-    val h = index(elemHashCode(key))
+  protected def findEntry(key: A): Entry =
+    findEntry0(key, index(elemHashCode(key)))
+
+  private[this] def findEntry0(key: A, h: Int): Entry = {
     var e = table(h).asInstanceOf[Entry]
     while (e != null && !elemEquals(e.key, key)) e = e.next
     e
@@ -138,7 +139,10 @@ trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashU
    *  pre: no entry with same key exists
    */
   protected def addEntry(e: Entry) {
-    val h = index(elemHashCode(e.key))
+    addEntry0(e, index(elemHashCode(e.key)))
+  }
+
+  private[this] def addEntry0(e: Entry, h: Int) {
     e.next = table(h).asInstanceOf[Entry]
     table(h) = e
     tableSize = tableSize + 1
@@ -147,6 +151,24 @@ trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashU
       resize(2 * table.length)
   }
 
+  /** Find entry with given key in table, or add new one if not found.
+   *  May be somewhat faster then `findEntry`/`addEntry` pair as it
+   *  computes entry's hash index only once.
+   *  Returns entry found in table or null.
+   *  New entries are created by calling `createNewEntry` method.
+   */
+  protected def findOrAddEntry[B](key: A, value: B): Entry = {
+    val h = index(elemHashCode(key))
+    val e = findEntry0(key, h)
+    if (e ne null) e else { addEntry0(createNewEntry(key, value), h); null }
+  }
+
+  /** Creates new entry to be immediately inserted into the hashtable.
+   *  This method is guaranteed to be called only once and in case that the entry
+   *  will be added. In other words, an implementation may be side-effecting.
+   */
+  protected def createNewEntry[B](key: A, value: B): Entry
+
   /** Remove entry from table if present.
    */
   protected def removeEntry(key: A) : Entry = {
@@ -195,7 +217,7 @@ trait HashTable[A, Entry >: Null <: HashEntry[A, Entry]] extends HashTable.HashU
   }
 
   /** Avoid iterator for a 2x faster traversal. */
-  protected def foreachEntry[C](f: Entry => C) {
+  protected def foreachEntry[U](f: Entry => U) {
     val iterTable = table
     var idx       = lastPopulatedIndex
     var es        = iterTable(idx)

src/library/scala/collection/mutable/LinkedHashMap.scala

@@ -67,23 +67,9 @@ class LinkedHashMap[A, B] extends AbstractMap[A, B]
   }
 
   override def put(key: A, value: B): Option[B] = {
-    val e = findEntry(key)
-    if (e == null) {
-      val e = new Entry(key, value)
-      addEntry(e)
-      updateLinkedEntries(e)
-      None
-    } else {
-      val v = e.value
-      e.value = value
-      Some(v)
-    }
-  }
-
-  private def updateLinkedEntries(e: Entry) {
-    if (firstEntry == null) firstEntry = e
-    else { lastEntry.later = e; e.earlier = lastEntry }
-    lastEntry = e
+    val e = findOrAddEntry(key, value)
+    if (e eq null) None
+    else { val v = e.value; e.value = value; Some(v) }
   }
 
   override def remove(key: A): Option[B] = {
@@ -143,38 +129,45 @@ class LinkedHashMap[A, B] extends AbstractMap[A, B]
       else Iterator.empty.next
   }
 
-  override def foreach[U](f: ((A, B)) => U) = {
+  override def foreach[U](f: ((A, B)) => U) {
     var cur = firstEntry
     while (cur ne null) {
       f((cur.key, cur.value))
       cur = cur.later
     }
   }
 
-  protected override def foreachEntry[C](f: Entry => C) {
+  protected override def foreachEntry[U](f: Entry => U) {
     var cur = firstEntry
     while (cur ne null) {
       f(cur)
       cur = cur.later
     }
   }
 
+  protected def createNewEntry[B1](key: A, value: B1): Entry = {
+    val e = new Entry(key, value.asInstanceOf[B])
+    if (firstEntry eq null) firstEntry = e
+    else { lastEntry.later = e; e.earlier = lastEntry }
+    lastEntry = e
+    e
+  }
+
   override def clear() {
     clearTable()
     firstEntry = null
   }
 
   private def writeObject(out: java.io.ObjectOutputStream) {
-    serializeTo(out, _.value)
+    serializeTo(out, { entry =>
+      out.writeObject(entry.key)
+      out.writeObject(entry.value)
+    })
   }
 
   private def readObject(in: java.io.ObjectInputStream) {
     firstEntry = null
     lastEntry = null
-    init[B](in, { (key, value) =>
-      val entry = new Entry(key, value)
-      updateLinkedEntries(entry)
-      entry
-    })
+    init(in, createNewEntry(in.readObject().asInstanceOf[A], in.readObject()))
   }
 }