Skip to content

Commit

Permalink
Add a toMap operation that returns a Map wrapper for HashMap
Browse files Browse the repository at this point in the history
  • Loading branch information
DavidGregory084 committed May 17, 2022
1 parent 6203e1a commit 0f66333
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 3 deletions.
28 changes: 28 additions & 0 deletions core/src/main/scala-2.12/cats/data/HashMapCompatCompanion.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package cats.data

import scala.collection.immutable.AbstractMap
import scala.util.hashing.MurmurHash3

private[data] trait HashMapCompatCompanion {
private[data] class WrappedHashMap[K, V](private[WrappedHashMap] val hashMap: HashMap[K, V]) extends AbstractMap[K, V] {
final def iterator: collection.Iterator[(K, V)] = hashMap.iterator
final def get(key: K): Option[V] = hashMap.get(key)
final def +[V1 >: V](kv: (K, V1)): Map[K, V1] = new WrappedHashMap(hashMap.updated(kv._1, kv._2))
final def -(key: K): Map[K, V] = new WrappedHashMap(hashMap.removed(key))
final override def size: Int = hashMap.size
final override def contains(key: K): Boolean = hashMap.contains(key)
final override def foreach[U](f: ((K, V)) => U): Unit = hashMap.foreach(Function.untupled(f))

This comment has been minimized.

Copy link
@johnynek

johnynek May 17, 2022

Contributor

I wonder if taking a Function2[K, V, A] in foreach is a mistake. Should we just use Function1[(K, V), A] like scala?

This comment has been minimized.

Copy link
@DavidGregory084

DavidGregory084 May 17, 2022

Author Member

I think that ergonomically ((K, V)) => A totally sucks, but I guess everyone is already used to that, and this way definitely does result in a lot of tupling and untupling

final override def getOrElse[V1 >: V](key: K, default: => V1): V1 = hashMap.getOrElse(key, default)
final override def keysIterator: Iterator[K] = hashMap.keysIterator
final override def valuesIterator: Iterator[V] = hashMap.valuesIterator
final override def isEmpty: Boolean = hashMap.isEmpty
final override def nonEmpty: Boolean = hashMap.nonEmpty
final override def hashCode: Int = MurmurHash3.mapHash(this)

This comment has been minimized.

Copy link
@johnynek

johnynek May 17, 2022

Contributor

why not hashMap.hashCode here?

This comment has been minimized.

Copy link
@DavidGregory084

DavidGregory084 May 17, 2022

Author Member

Yeah you're right, that would probably be fine, I guess nobody cares if a WrappedHashMap and a HashMap have the same hashcode.

This comment has been minimized.

Copy link
@DavidGregory084

DavidGregory084 May 17, 2022

Author Member

Or do they? Should we be passing in a seed to StaticMethods.unorderedHash like "cats.data.HashMap".hashCode instead of using the same seed as the Scala collections do?

final override def equals(that: Any): Boolean = that match {
case map: WrappedHashMap[_, _] =>
(this eq map) || (this.hashMap == map.hashMap)

This comment has been minimized.

Copy link
@johnynek

johnynek May 17, 2022

Contributor

doesn't equals on hashmap already do the eq check?

case other =>
super.equals(other)
}
}
}
31 changes: 31 additions & 0 deletions core/src/main/scala-2.13+/cats/data/HashMapCompatCompanion.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package cats.data

import scala.collection.immutable.AbstractMap
import scala.util.hashing.MurmurHash3

private[data] trait HashMapCompatCompanion {
private[data] class WrappedHashMap[K, V](private[WrappedHashMap] val hashMap: HashMap[K, V]) extends AbstractMap[K, V] {
final def iterator: collection.Iterator[(K, V)] = hashMap.iterator
final def get(key: K): Option[V] = hashMap.get(key)
final def updated[V1 >: V](key: K, value: V1): Map[K, V1] = new WrappedHashMap(hashMap.updated(key, value))
final def removed(key: K): Map[K, V] = new WrappedHashMap(hashMap.removed(key))
final override def size: Int = hashMap.size
final override def knownSize: Int = hashMap.size
final override def contains(key: K): Boolean = hashMap.contains(key)
final override def foreach[U](f: ((K, V)) => U): Unit = hashMap.foreach(Function.untupled(f))
final override def getOrElse[V1 >: V](key: K, default: => V1): V1 = hashMap.getOrElse(key, default)
final override def keysIterator: Iterator[K] = hashMap.keysIterator
final override def valuesIterator: Iterator[V] = hashMap.valuesIterator
final override def isEmpty: Boolean = hashMap.isEmpty
final override def nonEmpty: Boolean = hashMap.nonEmpty
final override def concat[V2 >: V](suffix: IterableOnce[(K, V2)]): Map[K, V2] =
new WrappedHashMap(hashMap.concat(suffix))
final override def hashCode: Int = MurmurHash3.mapHash(this)
final override def equals(that: Any): Boolean = that match {
case map: WrappedHashMap[_, _] =>
(this eq map) || (this.hashMap == map.hashMap)

This comment has been minimized.

Copy link
@johnynek

johnynek May 17, 2022

Contributor

same comments as above here.

case other =>
super.equals(other)
}
}
}
18 changes: 15 additions & 3 deletions core/src/main/scala/cats/data/HashMap.scala
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import cats.syntax.eq._
import java.util.Arrays

import HashMap.improve
import HashMap.WrappedHashMap

/**
* An immutable hash map using [[cats.kernel.Hash]] for hashing.
Expand Down Expand Up @@ -90,6 +91,14 @@ final class HashMap[K, +V] private[data] (private[data] val rootNode: HashMap.No
final def keysIterator: Iterator[K] =
iterator.map { case (k, _) => k }

/**
* An iterator for the values of this map that can be used only once.
*
* @return an iterator that iterates through the keys of this map.
*/
final def valuesIterator: Iterator[V] =
iterator.map { case (_, v) => v }

/**
* The size of this map.
*
Expand Down Expand Up @@ -177,6 +186,9 @@ final class HashMap[K, +V] private[data] (private[data] val rootNode: HashMap.No
new HashMap(newRootNode)
}

final def toMap: collection.immutable.Map[K, V] =
new WrappedHashMap(this)

/**
* Typesafe equality operator.
*
Expand Down Expand Up @@ -233,7 +245,7 @@ final class HashMap[K, +V] private[data] (private[data] val rootNode: HashMap.No
iterator.map { case (k, v) => s"$k -> $v" }.mkString("HashMap(", ", ", ")")
}

object HashMap extends HashMapInstances {
object HashMap extends HashMapInstances with HashMapCompatCompanion {
final private[data] def improve(hash: Int): Int =
scala.util.hashing.byteswap32(hash)

Expand Down Expand Up @@ -275,7 +287,7 @@ object HashMap extends HashMapInstances {
*
* @param iterable the iterable source of elements to add to the [[cats.data.HashMap]].
* @param hashKey the [[cats.kernel.Hash]] instance used for hashing values.
* @return a new [[cats.data.HashMap]] which contains all elements of `seq`.
* @return a new [[cats.data.HashMap]] which contains all elements of `iterable`.
*/
final def fromIterableOnce[K, V](iterable: IterableOnce[(K, V)])(implicit hashKey: Hash[K]): HashMap[K, V] = {
iterable match {
Expand All @@ -295,7 +307,7 @@ object HashMap extends HashMapInstances {
* @param fkv the [[cats.Foldable]] structure of elements to add to the [[cats.data.HashMap]].
* @param F the [[cats.Foldable]] instance used for folding the structure.
* @param hashKey the [[cats.kernel.Hash]] instance used for hashing values.
* @return a new [[cats.data.HashMap]] which contains all elements of `seq`.
* @return a new [[cats.data.HashMap]] which contains all elements of `fkv`.
*/
final def fromFoldable[F[_], K, V](fkv: F[(K, V)])(implicit F: Foldable[F], hashKey: Hash[K]): HashMap[K, V] = {
val rootNode = F.foldLeft(fkv, Node.empty[K, V]) { case (node, (k, v)) =>
Expand Down
8 changes: 8 additions & 0 deletions tests/shared/src/test/scala/cats/tests/HashMapSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -264,4 +264,12 @@ class HashMapSuite extends CatsSuite {
assertEquals(left, right)
}
}

property("toMap consistent with fromIterableOnce") {
forAll { (scalaMap: Map[Int, String]) =>
val hashMap = HashMap.fromIterableOnce(scalaMap)
val wrappedHashMap = hashMap.toMap
assertEquals(scalaMap, wrappedHashMap)
}
}
}

0 comments on commit 0f66333

Please sign in to comment.