You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
... but fails for a different invalid position, even if we don't actually remove an element ...
scala> lb.remove(9, 0)
java.lang.UnsupportedOperationException: tail of empty list
at scala.collection.immutable.Nil$.tail(List.scala:331)
at scala.collection.immutable.Nil$.tail(List.scala:326)
at scala.collection.mutable.ListBuffer.remove(ListBuffer.scala:283)
Invalid position + removing more items than existing, is OK again:
If position and amount of items to be removed == length, the operation succeeds, but the buffer is corrupted afterwards:
scala> lb.remove(4, 4)
scala> lb
java.util.NoSuchElementException: head of empty list
at scala.collection.immutable.Nil$.head(List.scala:329)
at scala.collection.immutable.Nil$.head(List.scala:326)
at scala.collection.mutable.ListBuffer$$anon$1.next(ListBuffer.scala:406)
at scala.collection.Iterator$$anon$10.next(Iterator.scala:312)
at scala.collection.Iterator$$anon$11.next(Iterator.scala:328)
at scala.collection.Iterator$class.foreach(Iterator.scala:727)
at scala.collection.AbstractIterator.foreach(Iterator.scala:1156)
at scala.collection.TraversableOnce$class.addString(TraversableOnce.scala:306)
at scala.collection.AbstractIterator.addString(Iterator.scala:1156)
at scala.collection.TraversableOnce$class.mkString(TraversableOnce.scala:272)
at scala.collection.AbstractIterator.mkString(Iterator.scala:1156)
at scala.runtime.ScalaRunTime$.scala$runtime$ScalaRunTime$$inner$1(ScalaRunTime.scala:323)
at scala.runtime.ScalaRunTime$.stringOf(ScalaRunTime.scala:332)
at scala.runtime.ScalaRunTime$.replStringOf(ScalaRunTime.scala:340)
The underlying cause of this is that the whole bounds-fixing code is highly questionable, especially because it isn't mentioned in the documentation at all:
overridedefremove(n: Int, count: Int) {
valn1= n max 0valcount1= count min (len - n1)
...
Imho we should kill this behaviour completely and replace it with appropriate bound checks throwing IOOBEs.
What do you think?
The text was updated successfully, but these errors were encountered:
@odersky said:
I agree. ListBuffer.remove breaks the contract established by BufferLike.remove, which requires bounds checks. It's also inconsistent with the unary version of remove in ListBuffer. We just have to make sure that ListBuffer.remove's broken behavior is not assumed elsewhere in the libraries.
@soc said:
As far as I see, there is only one alternative usage: People who used the remove method to say "remove all elements starting from this index", e. g. index valid, count too large:
vallb= collection.mutable.ListBuffer('a, 'b, 'c, 'd, 'e)
lb remove (4, Int.MaxValue) // "Remove all items starting from index 4"
I would probably just rule out this usage (I checked ArrayBuffer and UnrolledBuffer and both don't have this behvaiour, too) and – only if people actually complain – add a special method for this use case similar to trimStart/trimEnd (maybe trimFrom?).
@retronym said:
Simon, I'm closing this ticket on the assumption that we forgot to do so after your patch was merged. Please reopen or open a new ticket if there is a residual problem that I've missed.
@soc said:
Mhhh, as far as I remember the ticket was open to remind us that we need to properly specify the behavior in error cases and check that all collections conform to this behavior.
There is a off-by-one error in the computation of
count1
, which fails to trigger an exception in the following case:Removing one item from an invalid position where position == length works ...
... but fails for a different invalid position, even if we don't actually remove an element ...
Invalid position + removing more items than existing, is OK again:
If position and amount of items to be removed == length, the operation succeeds, but the buffer is corrupted afterwards:
The underlying cause of this is that the whole bounds-fixing code is highly questionable, especially because it isn't mentioned in the documentation at all:
Imho we should kill this behaviour completely and replace it with appropriate bound checks throwing IOOBEs.
What do you think?
The text was updated successfully, but these errors were encountered: