Port/implement j.u.ArrayDeque #1696
Conversation
Use ArrayDeque from recent SN PR scala-native#1696 to match the changes in the re2j PR. Eliminating SN divergence should make it easier to merge some known re2j changes planned for two or more rounds out.
LeeTibbert
force-pushed
the
PR_j_u_ArrayDeque
branch
from
958a9bd
to
d3fa97c
Compare
|
preemptively rebased. Travis CI is all green. |
| /// * The order of method declarations is not alphabetical to reduce | ||
| /// churn versus ScalaJs original. | ||
|
|
||
| class ArrayDeque[E] private (private var inner: ArrayList[E]) |
There was a problem hiding this comment.
inner is never reassigned. It could and should be a val.
There was a problem hiding this comment.
Done. Good catch.
|
|
||
| def pollLast(): E = { | ||
| if (inner.isEmpty) null.asInstanceOf[E] | ||
| else inner.remove(inner.size - 1) |
There was a problem hiding this comment.
The else branch should also increase the status, like in pollFirst().
|
|
||
| private def failFastIterator(startIndex: Int, nex: (Int) => Int) = { | ||
| new Iterator[E] { | ||
| private def checkStatus() = |
There was a problem hiding this comment.
A method body spanning several lines should be enclosed in {}.
There was a problem hiding this comment.
Done, artifact of original code.
| // ArrayList.spliterator is not yet implemented. | ||
| // def spliterator(): Spliterator[E] = inner.spliterator() |
There was a problem hiding this comment.
In general, since spliterators are not implemented anywhere, I recommended not even mentioning it here, even commented. If we one day add spliterators, we'll have to implement them everywhere anyway, in a holistic fashion.
| // def spliterator(): Spliterator[E] = inner.spliterator() | ||
|
|
||
| override def toArray(): Array[AnyRef] = { | ||
| inner.toArray(new Array[AnyRef](size)) |
There was a problem hiding this comment.
| inner.toArray(new Array[AnyRef](size)) | |
| inner.toArray() |
There was a problem hiding this comment.
Done. The replacement is more focused and concise. Thank you.
| * | ||
| * See the NOTICE file distributed with this work for | ||
| * additional information regarding copyright ownership. | ||
| */ |
There was a problem hiding this comment.
You can remove that header and just have a comment // Ported from Scala.js, like all the other files that have been ported from Scala.js.
| /// * equals() & hashcode() are not implemented so comparing ArrayDeque | ||
| /// instances ad1 == ad2 is bit of a pain. It gives object equality | ||
| /// not the more useful contents equality. Someday... | ||
| /// https://alvinalexander.com/scala/ \ | ||
| /// how-to-define-equals-hashcode-methods-in-scala-object-equality |
There was a problem hiding this comment.
equals and hashCode are inherited from AbstractList, which has a correct implementation. This comment is misleading and should be removed.
There was a problem hiding this comment.
Done, thank you for the insight & correction.
| /// * spliterator() is implemented but commented out because the | ||
| /// ArrayList.spliterator it delegates to is not yet implemented. |
There was a problem hiding this comment.
This can be removed together with the commented out definition.
* This PR ports java.util.ArrayDeque from Scala.js and extensively
modifies it for ScalaNative.
My thanks and appreciation to @sjrd and the Scala.js team for
the original code. As a token of that appreciation, I hope to
backfile a PR for a slight deviation in Scala.js from JVM
behavior in AbstractCollection.toString.
All bugs introduced and misbehaviors are my original contribution.
* The test suite ArrayDequeSuite.scala was created.
One test is marked `testFails`. See SN Issue scala-native#1694
"j.l.System.arraycopy should more closely match JVM; missing
ArrayStoreExceptions" for details. It is not evident how
the failing test relates to Issue scala-native#1694, but it a week or so
of tracing will show that it does.
Documentation:
* The standard changelog entry is requested.
Testing:
* Built and tested ("test-all") in release-fast mode using sbt 1.2.8 on
X86_64 only . All tests pass.
LeeTibbert
force-pushed
the
PR_j_u_ArrayDeque
branch
from
d3fa97c
to
7cb6211
Compare
|
FYI: I documented at the top of ArrayDeque.scala that the change in Scala.js to the file |
Use ArrayDeque from recent SN PR scala-native#1696 to match the changes in the re2j PR. Eliminating SN divergence should make it easier to merge some known re2j changes planned for two or more rounds out.
This commit ports java.util.ArrayDeque from Scala.js and extensively modifies it for ScalaNative. One test is marked `testFails`. See SN Issue scala-native#1694 "j.l.System.arraycopy should more closely match JVM; missing ArrayStoreExceptions" for details.
Use ArrayDeque from recent SN PR scala-native#1696 to match the changes in the re2j PR. Eliminating SN divergence should make it easier to merge some known re2j changes planned for two or more rounds out.
This PR ports java.util.ArrayDeque from Scala.js and extensively
modifies it for ScalaNative.
My thanks and appreciation to @sjrd and the Scala.js team for
the original code. As a token of that appreciation, I hope to
backfile a PR for a slight deviation in Scala.js from JVM
behavior in AbstractCollection.toString.
All bugs introduced and misbehaviors are my original contribution.
The test suite ArrayDequeSuite.scala was created.
One test is marked
testFails. See SN Issue j.l.System.arraycopy should more closely match JVM; missing ArrayStoreExceptions #1694"j.l.System.arraycopy should more closely match JVM; missing
ArrayStoreExceptions" for details. It is not evident how
the failing test relates to Issue j.l.System.arraycopy should more closely match JVM; missing ArrayStoreExceptions #1694, but it a week or so
of tracing will show that it does.
Documentation:
Testing:
X86_64 only . All tests pass.