Don't extend collection types in ArrayOps #479
Conversation
All collection-related methods for arrays are available on WrappedArray so there is no need to duplicate them in ArrayOps. This allows ArrayOps to be implemented as a value class with no inherited methods, thus it should never be necessary to allocate an instance of it. Unlike in the old collection library `ArrayOps` is not specialized. The methods that were specialized before don’t seen worth specializing. In particular, `apply` and `update` which *should* be specialized on arrays are not provided at all by the new `ArrayOps` because they are available natively on `Array`. The old specialization scheme would have no benefit but a high cost: it is incompatible with a pure value class. Almost any ArrayOps call in the old library needs to allocate an instance, with the only exceptions being `length`, `apply` and `update`. New and optimized implementations of many collection methods are provided in ArrayOps, often adapted from Slick’s `ConstArray` class.
- `WrappedArray` and `ImmutableArray` can implement it with `Array.copy`. - In `ArrayBuilder.addAll` we do not need to take specialization of `ImmutableArray` into account. The old implementation used `Array.copy` which falls back to a slow copy when the element types don’t match. This is still faster than the primitive implementation in `super.addAll` and `Iterator.copyToArray` as the last resort at least should not be worse. The new `addAll` based on `copyToArray` works well with all collection types that implement `copyToArray` efficiently. - `prependedAll` and `appendedAll` in both, `ImmutableArray` and `ArrayOps` also do not need to optimize for known `ImmutableArray` arguments anymore.
|
Nice! What are your thoughts for keeping the "interface" (available methods) in synch? #465 |
|
This PR makes type inference fail on arrays usage. Also, what do you think of implementing the optimized versions of reduction operations on |
|
There's one type inference failure in the tests. I have not been able to figure out why this change makes it fail. The example we have is something like |
|
We could additionally make |
|
Something weird is going on with the The problem only occurs when testing in the strawman namespace. My guess would be that some special case handling in the compiler is getting in the way. For example, you can see in |
|
Ticket for the overload problem: scala/bug#10746 The reason why it works from Predef is that we still have the separate implicits for converting primitive and reference arrays to |
szeiger
force-pushed
the
wip/collection-less-arrayops
branch
from
2295fd1 to
f5041c6
Compare
szeiger
force-pushed
the
wip/collection-less-arrayops
branch
from
f5041c6 to
3560a69
Compare
|
|
||
| private def mkArray(size: Int): Array[T] = { | ||
| val newelems = new Array[T](size) | ||
| // Anything short of this will make Dotty create an Array[Object]: | ||
| val newelems = java.lang.reflect.Array.newInstance(ct.runtimeClass, size).asInstanceOf[Array[T]] |
There was a problem hiding this comment.
This is a strange one. I was unable to come up with a reproduction in the REPL but it fails consistently here. When calling ArrayOps.zip the ClassTag is the one for Tuple2 but I end up with an Array[Object] when I use new Array or Array.ofDim (even passing the ClassTag explicitly).
There was a problem hiding this comment.
I opened scala/scala3#4037
| /** A copy of this array with an element appended. */ | ||
| def appended[B >: A : ClassTag](x: B): Array[B] = { | ||
| val dest = new Array[B](xs.length + 1) | ||
| Array.copy(xs, 0, dest, 0, xs.length) |
There was a problem hiding this comment.
@mkeskells suggests using Arrays.copyOf(xs, xs.length+1, classTag[B]) (scala/scala#6105 (comment))
There was a problem hiding this comment.
Good idea. We can also optimize all slice-based operations with copyOfRange.
There was a problem hiding this comment.
Do you think it's possible to optimize for the case where B is A, could we have an overload that avoids materializing the ClassTag?
See commit comments for details.