SI-8553 - WrappedArray throws exception on lastIndexWhere when index out of range

[Rui Gonçalves]

Hi,

I'm thinking about submitting a pull request for this issue: https://issues.scala-lang.org/browse/SI-8553.

The given example, `(Array(2): collection.mutable.WrappedArray[Int]).lastIndexWhere(_ => true, 1)`, should return 0, which is the last index of the collection. I identified the problem as being on `IndexedSeqOptimized`, where `lastIndexWhere` tries to access the element at the `end` index even if it is out of bounds. The fix seems easy and should not have any performance impact nor cause binary incompatibility.

May I submit the fix accompanied by a simple unit test?

[Som Snytt]

That's a fun one.  As a quotidian example,

scala> "abc123".lastIndexWhere(_.isLetter, 6)
java.lang.StringIndexOutOfBoundsException: String index out of range: 6

I guess you mean that the ticket should say that graceful handling means search from the last element.

Does the fix involve `lengthCompare`?  I see that on `IndexedSeqOptimized` it's defined in terms of length.  But what if length is unknown and large, but end is small.  Just probing `seq(end)` could be cheaper than `seq.length`.

I noticed that `SeqLike.lastIndexWhere` relies on the reverseIterator either being finite or producing an element that doesn't satisfy the predicate (because it doesn't check index >= 0).  OK, I see that both reverseIterator and length have warnings about not terminating.  The warning about not terminating would be a doc fix for `lastIndexWhere`.

(If reverseIterator returns an iterator it must have length elements, presumably.)

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Rui Gonçalves]

Yes, the example in the issue is not caused by not existing any element for which the predicate is true, but rather for the unchecked `end`.

I would replace `var i = end` with `var i = end min self.length - 1`. I don't see any way we can avoid using `length` here because we are not only interested to know if `end` is a valid index, but also to know the last valid index if `end` is not. If the collection has infinite size, the `lastIndexWhere` method may not make sense at all.

I see that `last` already relies on length - 1. Isn't it reasonable to assume that in finite indexed sequences the length of the sequence can be computed in contant time most probably?

[Rui Gonçalves]

I submitted the pull request now, it's on https://github.com/scala/scala/pull/3743. Please give it a look and share your opinions.

[Rui Gonçalves]

Hi again,

In the Pull Request Policy page (http://docs.scala-lang.org/scala/pull-request-policy.html) I read this: "Please note: you are responsible for meeting these criteria (reminding your reviewer, for example)". Do I have to contact a reviewer in order for the PR to be considered, or is it just a question of waiting? I'm new to contributing to Scala and I'm just being sure that I did what it needed to be done :)

[Adriaan Moors]

We try to be pro-active in assigning reviewers when the PR doesn't mention one, so this is mostly a disclaimer and encouragement to go look for someone who worked on similar stuff and nominate them as your reviewer :)

JOIN US. REGISTER TODAY!

Scala

Days

June 16th-18th,

Berlin

[Som Snytt]

Usual human limits apply, but you can nudge someone like @ Rex, or if it's not critical to you, just wait for the street sweeper.  The Team is sensitive to PR queue length.