Skip to content

SI-7128 copyToArray(xs, 0, 0) should not fail#4098

Merged
retronym merged 1 commit intoscala:2.12.xfrom
retronym:ticket/7128
Jan 27, 2015
Merged

SI-7128 copyToArray(xs, 0, 0) should not fail#4098
retronym merged 1 commit intoscala:2.12.xfrom
retronym:ticket/7128

Conversation

@retronym
Copy link
Member

@retronym retronym commented Nov 6, 2014

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.

@retronym
Copy link
Member Author

retronym commented Nov 6, 2014

Retargetting @Ichoran's #3940 against 2.12.x, as discussed. Review by @lrytz

@retronym
Copy link
Member Author

retronym commented Nov 6, 2014

Rebased on 2.12.x to avoid the hanging test.

@retronym
Copy link
Member Author

retronym commented Nov 7, 2014

A checkfile also needed updating, for that, re-review please by @Ichoran or @lrytz

@lrytz
Copy link
Member

lrytz commented Nov 7, 2014

Are the two ArrayIndexOutOfBoundsException in the test expected? To me they make sense, so LGTM.

@retronym
Copy link
Member Author

retronym commented Nov 7, 2014

Arguably the error message has regressed a little, but that seemed like an intentional change. I'll defer to @Ichoran on that.

@som-snytt
Copy link
Contributor

I noticed recently that I urged a rich error message. But I defer to deference.

Currently, iterator.copyToArray(a, 1, Int.MaxValue) does something, but will do nothing with this change.

@Ichoran
Copy link
Contributor

Ichoran commented Nov 7, 2014

@som-snytt - Good catch! The easiest fix would be to drop a .toLong on start and .toInt the min.

@lrytz
Copy link
Member

lrytz commented Nov 10, 2014

@lrytz lrytz removed the reviewed label Nov 10, 2014
@Ichoran
Copy link
Contributor

Ichoran commented Nov 24, 2014

@lrytz - Yes, that would do the trick. @retronym - I don't have access to make this fix any more. Have you got it?

@retronym
Copy link
Member Author

retronym commented Jan 8, 2015

@som-snytt @Ichoran I've shored up the overflow behaviour with a test, which passes after following @lrytz's suggestion.

@retronym
Copy link
Member Author

retronym commented Jan 8, 2015

@Ichoran For future reference, you could have submitted a pull request to retronym/ticket/7128 to contribute to this PR, or alternatively closed it an opened another.

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.
@retronym
Copy link
Member Author

Ping @Ichoran for re-review.

@Ichoran
Copy link
Contributor

Ichoran commented Jan 22, 2015

LGTM

retronym added a commit that referenced this pull request Jan 27, 2015
SI-7128  copyToArray(xs, 0, 0) should not fail
@retronym retronym merged commit 8b5f2b4 into scala:2.12.x Jan 27, 2015
@adriaanm adriaanm added the 2.12 label Oct 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants