This repository has been archived by the owner on Dec 22, 2021. It is now read-only.
Various Map implementations ported from Scala 2.12 #334
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I can see the usefulness of these collections, but couldn’t they stay in the compiler? Or is it possible to publish them in a separate artifact? |
|
They are part of the standard library now. If we don't want to break stuff unnecessarily they should stay where they are. |
|
We have already moved several parts of the standard library into separate modules. We could do the same here. |
szeiger
force-pushed
the
wip/exotic-maps
branch
from
3f183f8 to
2e14651
Compare
|
I made some changes for Dotty source compatibility, now it fails with a compiler error on Dotty: |
|
Opened scala/scala3#3775 for this |
|
You can make dotty happy by defining |
szeiger
force-pushed
the
wip/exotic-maps
branch
from
2e14651 to
6fdf770
Compare
|
@allanrenucci Thanks, that does the trick. @julienrf I suggest we look into possible further modularization after merging back into scala/scala. Right now I'd like to get these changes merged quickly so I can reduce the diff of my integration branches (which is thousands of loc for both collection-strawman and scala) |
julienrf
reviewed
Jan 9, 2018
| trait DefaultMap[K, +V] extends Map[K, V] { self => | ||
|
|
||
| // Members declared in IterableOps | ||
| def iterableFactory: IterableFactoryLike[Iterable] = Iterable |
There was a problem hiding this comment.
You could even refine the return type to IterableFactory[Iterable]
There was a problem hiding this comment.
I wonder if we need IterableFactoryLike at all. The only use is in View where automatic buffering would be a sensible alternative with a simpler API.
There was a problem hiding this comment.
We could try something like that, indeed.
|
|
||
| def map[V2](f: ((Int, T)) => (Int, V2)): IntMap[V2] = intMapFromIterable(View.Map(toIterable, f)) | ||
|
|
||
| def flatMap[V2](f: ((Int, T)) => IterableOnce[(Int, V2)]): IntMap[V2] = intMapFromIterable(View.FlatMap(toIterable, f)) |
There was a problem hiding this comment.
For the sake of completeness you might also want to add an overload for concat. And you probably want to refine withFilter as well.
There was a problem hiding this comment.
I'll add concat. I was unable to implement withFilter without triggering the same compiler bug I ran into earlier (when WithFilter was still a trait, IIRC, which is what it has to be in order to get the types right in these cases). I'll try to get back to this at some point and remove the path-dependent WithFilter types.
| with StrictOptimizedIterableOps[(Long, T), Iterable, LongMap[T]] { | ||
|
|
||
| protected[this] def fromSpecificIterable(coll: strawman.collection.Iterable[(Long, T)]): LongMap[T] = { | ||
| //TODO should this be the default implementation of this method in StrictOptimizedIterableOps? |
There was a problem hiding this comment.
Yes, probably, that’s a good point. I’ve noticed that we could probably provide more default implementations of the factory methods in the Ops traits in general. I’ve created #335 to keep track of that.
| b.addAll(coll) | ||
| b.result() | ||
| } | ||
| def iterableFactory: IterableFactoryLike[Iterable] = Iterable |
There was a problem hiding this comment.
I would refine the return type to IterableFactory[Iterable], here too.
| arm | ||
| } | ||
|
|
||
| /*override*/ def updated[V1 >: V](key: K, value: V1): AnyRefMap[K, V1] = { |
There was a problem hiding this comment.
Indeed we don’t have an updated operation anymore on mutable Maps. We should probably deprecate that one, what do you think?
There was a problem hiding this comment.
Is there a good replacement? updated can widen the value type which otherwise requires a cast. The scalac codebase doesn't use the method though.
There was a problem hiding this comment.
The replacement is myMutableMap.toMap.updated(k, v).to(AnyRefMap)
There was a problem hiding this comment.
This would never be a suitable replacement because it's horribly slow. If you didn't care about performance you wouldn't be using an AnyRefMap in the first place.
There was a problem hiding this comment.
I think I've found a solution: I'll add AnyRefMap.from. The object can't implement any of the standard factories, so this method doesn't exist yet, but it's a natural place to perform a widening copy and we can easily optimize the case where you're copying an existing AnyRefMap. With this method m.updated[V1](k,v) becomes { val m2 = AnyRefMap.from[K, V1](m); m2 += ((k,v)); m2 }.
| } | ||
|
|
||
| /** Adds a new key/value pair to this map and returns the map. */ | ||
| def +=(key: Long, value: V): this.type = { update(key, value); this } |
There was a problem hiding this comment.
This looks weird, why don’t we take a tuple as parameter?
There was a problem hiding this comment.
To avoid boxing and tupling. That's two unnecessary allocations per call.
There was a problem hiding this comment.
To clarify further: This is an overload. LongMap inherits the tupled and boxed version from Growable.
| @@ -53,6 +53,7 @@ trait Map[K, V] | |||
| trait MapOps[K, V, +CC[X, Y] <: MapOps[X, Y, CC, _], +C <: MapOps[K, V, CC, C]] | |||
| extends IterableOps[(K, V), Iterable, C] | |||
| with collection.MapOps[K, V, CC, C] | |||
| with Growable[(K, V)] | |||
szeiger
force-pushed
the
wip/exotic-maps
branch
from
6fdf770 to
3f43a71
Compare
julienrf
approved these changes
Jan 9, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another big chunk of my scala-integration branch that contains some of the more exotic
Maptypes from Scala 2.12. They are all used in the compiler codebase.