SI-7128 copyToArray(xs, 0, 0) should not fail#3940
SI-7128 copyToArray(xs, 0, 0) should not fail#3940Ichoran wants to merge 1 commit intoscala:2.11.xfrom
Conversation
Fixed all copyToArray methods to do exactly what the docs say they do, with the least-suprise behavior of not throwing an exception if you ask to copy nothing (but would have copied out of range). Iterator had an undocumented requirement for the target index to be in range regardless if anything happened; this has been removed.
|
Same question as in #3941: should we add unit tests for specific fixes? |
|
I don't think there's much point leaving tests for specific fixes because the quasi-comprehensive tests catch these. However, we should be sure that the changed behavior of t6827 is okay: it's throwing ArrayOutOfBoundsException instead of IllegalArgumentException when indexing to negative numbers, and if you try to copy off the end of the array there's no exception (even if you copy nothing) because the API says you stop when you reach the end of the array. I think this is the correct behavior, but I would like a LGTM from someone else before updating the check file. These are the lines that return different results: object Test extends App {
val ns = (0 until 20)
val arr = new Array[Int](10)
def tryit(label: String, start: Int, len: Int): Unit = {
val status = try {
val it = ns.toIterator
it.copyToArray(arr, start, len)
"ok"
} catch {
case e: Exception => e.toString
}
println("%s: %s" format (label, status))
}
tryit("start at -5", -5, 10) // Changed exception type
tryit("start at -1", -1, 10) // Changed exception type
tryit("start at limit", 10, 10)
tryit("start at limit-1", 9, 10)
tryit("first 10", 0, 10)
tryit("read all", 0, 20)
tryit("test huge len", 0, Int.MaxValue)
tryit("5 from 5", 5, 10)
tryit("20 from 5", 5, 20)
tryit("test len overflow", 5, Int.MaxValue)
tryit("start beyond limit", 30, 10) // New behavior--okay, does nothing
tryit("read 0", 0, 0)
tryit("read -1", 0, -1)
tryit("invalid read 0", 30, 0) // New behavior--okay, does nothing because zero elements get copied
tryit("invalid read -1", 30, -1) // New behavior--Also okay because zero elements get copied
// okay, see SI-7128
"...".toIterator.copyToArray(new Array[Char](0), 0, 0) // (I can update this line)
} |
|
Ping @lrytz - does the updated behavior look okay to you? (And is it okay to rely upon Q.C.C.T. for testing?) |
|
The changed behavior looks good to me, but I'm in favor of submitting this against 2.12.x. For testing, I think relying on collection-lawas is fine. |
|
Closing for resubmission to 2.12.x. |
Fixed all copyToArray methods to do exactly what the docs say they do, with the least-suprise behavior of not throwing an exception if you ask to copy nothing (but would have copied out of range).
Iterator had an undocumented requirement for the target index to be in range regardless if anything happened; this has been removed.