Floor for NewVectorIterator.slice limit

[kapunga]

Fixes scala/bug#12823
Adds a floor of 0 to the limit parameter of NewVectorIterator.slice. This matches the base behavior of Iterator.slice and Vector.slice.

[ansvonwa]

Looks good to me and seems pretty solid against me playing around with it.
Two things I noted:

• The infix max isn't used anywhere else in Vector.scala, so using the imported mmax (mmax(until, 0)) may be more consistent. (And maybe save one allocation? not sure..)
• The slice method leaves the remaining iterator in a different state than Iterator.slice does. But as the Documentation of Iterator clearly states "one should never use an iterator after calling a method on it" this seems fine.

[Ichoran]

It looks fine to me. I would tend to favor more direct logic, more like:

val n =
  if (until <= 0 || from >= until) 0
  else if (from > 0) {
    drop(from)
    until - from
  }
  else until
take(n)

but the current version seems to do the same thing, just in a slightly harder-to-parse way (and doing a bit more computation in the from > until case).

[He-Pin]

if (until<= 0 || until<= from)  Iterator.empty else {
 if (from > 0) drop(from)
 take(if(from < 0) until else until- from)
}

[ansvonwa]

I'm unsure about your comment. Is this a third proposal? If so, I think it should be until instead of util, and there are some formatting and correctness issues (for instance, .slice(0,0) returns the entire vector, so this would need to be e.g. Iterator.empty).

[He-Pin]

@ansvonwa yes, I edit it on my phone, thanks for point out, but I think you can update your pr to avoid the usage of max. and @Ichoran's proposal is very direct too.

[kapunga]

• The infix max isn't used anywhere else in Vector.scala, so using the imported mmax (mmax(until, 0)) may be more consistent. (And maybe save one allocation? not sure..)

Makes sense to keep things consistent. I'll change this.

• The slice method leaves the remaining iterator in a different state than Iterator.slice does. But as the Documentation of Iterator clearly states "one should never use an iterator after calling a method on it" this seems fine.

I agree that it's fine. The original PR for this file clearly had a lot of thought put into performance, I'm hesitant to make changes for changes sake and limited this PR to just fixing the immediate bug.

[He-Pin]

I think the most quick fix can be mmax(util, 0), inline it here, otherwise lgtm.

[ansvonwa]

I'm hesitant to make changes for changes sake and limited this PR to just fixing the immediate bug.

Definitely a good habit.
For completeness: The different state only occurs in out of bounds cases, namely from > until. There, your solution still performs the drop while @Ichoran's and the default Iterator don't. Bus as said, this should be fine.

I wonder how much would break if each such operation on an Iterator would set it to a "dirty" state where every future operation throws 😈

[SethTisue]

@kapunga Looks like this is ready for merge — but before we do that, can you squash the two commits into one? (We don't squash-merge in this repo.)

[kapunga]

@kapunga Looks like this is ready for merge — but before we do that, can you squash the two commits into one? (We don't squash-merge in this repo.)

@SethTisue done!

[SethTisue]

thank you @kapunga and reviewers!

[som-snytt]

The merge has a red ex ❌ presumably unrelated to the PR.

 [error] Test scala.collection.IteratorTest.flatMap is memory efficient in previous element failed: org.junit.ComparisonFailure: expected:<[first]> but was:<[third]>, took 0.011 sec
[error]     at scala.collection.IteratorTest.flatMap is memory efficient in previous element(IteratorTest.scala:842)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error]     at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error]     at java.lang.reflect.Method.invoke(Method.java:568)
[error]     ...

It looks like the test relies on a weak reference and is prone to flake.