-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IndexedSeq#head
now throws NoSuchElementException
(not IndexOutOfBoundsException
)
#10392
Conversation
override def head: A = | ||
try apply(0) | ||
catch { | ||
case e: IndexOutOfBoundsException => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work on Scala.js. Array index out of bounds is undefined behavior over there. Besides, using a try
for the happy path is not quite as free as on the JVM.
It seems to me that an explicit isEmpty
test would be more appropriate. It's what we have in other collections I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I waffled on isEmpty
because of hidden costs, but maybe that is paranoid for an indexed thing. Looking at it now, I see the PR from last year was that IterableOnce#isEmpty
now uses knownSize
first and iterator.hasNext
second. I haven't looked whether indexed implies size is known, but it seems likely. They have to override something useful if they want a useful collection.
Edit: I haven't internalized that IndexedSeq
means "efficient apply
and length
" and knownSize
is just length
. Also SeqOps#isEmpty
is lengthCompare(0)
, so probably isEmpty
is guaranteed to be fine.
IndexOutOfBoundsException
doesn't imply ArrayIndexOutOfBounds
; this behavior is only for collections which don't override head
. I didn't quite do a survey but picked ArrayDeque
as an example, and it happens to do an explicit bounds check.
"Ideal" would be a comprehensive unit and benchmark test for performance and coverage (which collections or wrappers fail to do something specific for head
or isEmpty
?). I said I didn't benchmark last year, so I'll try that.
Also meant to add, I figured out how to invoke
sbt:root> junit/testOnly scala.collection.IndexedTestImpl.ArrayBufferTest
but the types aren't constrained where you can toss in a test of xs.head
, so I gave up quickly. Also in progress is a tweak or update to collections-laws
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to document that the first attempt was new NSEE(ioobe.getMessage, ioobe.getCause)
but actually the new exception should have had the NSEE
as the cause.
Seq.isEmpty is lengthCompare and IndexedSeq has efficient length aka knownSize. headOption already uses isEmpty.
I caved to sjrd's reasonable demands. The code is better but the PR title is much more ordinary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This looks good to me. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -91,11 +91,25 @@ trait IndexedSeqOps[+A, +CC[_], +C] extends Any with SeqOps[A, CC, C] { self => | |||
|
|||
override def slice(from: Int, until: Int): C = fromSpecific(new IndexedSeqView.Slice(this, from, until)) | |||
|
|||
override def head: A = apply(0) | |||
override def head: A = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this now lead to repeated checks for isEmpty
in {head,last}Option
? should there be an @inline def headImpl: A = apply(0)
and used in both head
and headOption
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment on the first commit is that head is fast. headOption
is allocating.
Release-noting since there are compatibility implications here — for those catching exceptions, and for those extending |
IndexedSeq#head
throws NoSuchElementException
(not IndexOutOfBoundsException
as before)
IndexedSeq#head
throws NoSuchElementException
(not IndexOutOfBoundsException
as before)IndexedSeq#head
now throws NoSuchElementException
(not IndexOutOfBoundsException
)
Fixes scala/bug#12782
Attempts to use
collectionClassName
for a helpful exception message, falls back totoString
, which should be fine if the collection is in fact empty. (Extra parens at worst.)AvoidsRelishesisEmpty
in the happy path.