ArrayBuilder of Unit needs addAll forwarded #10958
Conversation
|
@scala/collections |
SethTisue
added
the
library:collections
|
Added to current milestone not for pragmatic reasons, as the alternative method works, but just because. Quick review: did you add a test for the other method? Edit: There was already a test that it throws...! |
|
does I'm the problem live in 2.12.x too？ |
|
@He-Pin No, this is 2.13.x with new collections problems. |
|
I need the meme with the fellow with the nice beard: I don't always throw exceptions, but when I do, I make it a |
|
I absolutely did run Therefore, the task as it currently exists is not useful to me locally. If there is some hoop to jump through, it is probably also not useful to me. |
som-snytt
force-pushed
the
issue/13068-ary-bld
branch
from
8d53f74 to
c54172b
Compare
| val ab: ArrayBuilder[Unit] = ArrayBuilder.make[Unit] | ||
| val arr = Array[Unit]((), (), (), (), (), (), (), (), (), (), (), ()) | ||
| ab.addAll(arr) | ||
| assertEquals(arr.length, ab.result().length) |
There was a problem hiding this comment.
Should we assert the elements are all ()
There was a problem hiding this comment.
That sounds extreme, but result doesn't use Array.fill. That is an "optimization", but also therefore a potential point of failure. This class "must" be rarely used, so arguably any optimization is not worth it. Look at the time suck due to "optimizing" the range checks. Probably the test should verify the result.
There was a problem hiding this comment.
Thanks for the input, but verify result seems ✅️
There was a problem hiding this comment.
but then I would have to decide whether to nowarn
[warn] .../ArrayBuilderTest.scala:48:37: dubious usage of method == with unit value
[warn] assertTrue(ab.result().forall(_ == ()))
[warn] ^
There was a problem hiding this comment.
but then I would have to decide whether to nowarn
[warn] .../ArrayBuilderTest.scala:48:37: dubious usage of method == with unit value [warn] assertTrue(ab.result().forall(_ == ())) [warn] ^
Maybe eq？
There was a problem hiding this comment.
oh good suggestion. I did add a commit earlier, but didn't push it yet, because I feel bad for Jenkins.
som-snytt
changed the title
|
I restored the forwarding |
| @@ -46,7 +46,7 @@ sealed abstract class ArrayBuilder[T] | |||
| protected[this] def resize(size: Int): Unit | |||
|
|
|||
| /** Add all elements of an array. */ | |||
| def addAll(xs: Array[_ <: T]): this.type = doAddAll(xs, 0, xs.length) | |||
| def addAll(xs: Array[_ <: T]): this.type = addAll(xs, 0, xs.length) | |||
There was a problem hiding this comment.
We can just forwarding when xs is not empty
There was a problem hiding this comment.
That is a micro-optimization too far.
|
It was green until I tempted fate with another Travis build: which looks like "ten months". |
Fixes scala/bug#13068
Follow-up to previous changes for resizing.
One
addAllused to forward to the other. That changed just to avoid redundant range checks.