Skip to content
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

RubyArray.subList should treat toIndex as exclusive, not inclusive. #1274

Closed
masover opened this Issue Nov 26, 2013 · 2 comments

Comments

Projects
None yet
2 participants
@masover
Copy link

masover commented Nov 26, 2013

That is, if I pass a ruby array to something expecting a list, and it calls myArray.subList(0,1), it should receive a one-element sublist, not a two-element sublist. Similarly, myArray.subList(0, myArray.size()) should return the entire array, even though size() is one past the last valid index.

The source for subList includes:

IRubyObject subList = subseq(fromIndex, toIndex - fromIndex + 1);

It looks like subseq expects an offset and a length. However, from the Java API docs:

Returns a view of the portion of this list between the specified fromIndex, inclusive, and toIndex, exclusive.

So the length of that range should be toIndex - fromIndex. We can see this in the behavior of such a list:

public static void testList(List<?> items) {
  System.out.println("Received " + items.size() + " items");
  System.out.println("sublist(0,1).size() is " + items.subList(0,1).size());
}

Call this with an ArrayList, and it gives a sublist of size 1. Call it from JRuby with a Ruby array, and it gives a sublist of size 2. (A workaround in JRuby is to explicitly construct an ArrayList and addAll() the Ruby array, but that's silly.)

To fix: Just change it to:

IRubyObject subList = subseq(fromIndex, toIndex - fromIndex);

I'll generate a pull request if it really helps, but this should be a one-liner. Even the bounds check is correct (it allows toIndex==size() (should return a sublist ending at the end of the list), and it allows fromIndex==toIndex (should return an empty sublist)).

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 27, 2013

I can certainly make the change, but perhaps you'd like to do the PR, plus add a test to our Java based tests under core/src/test/java? We'll need to add a test in either case.

masover pushed a commit to masover/jruby that referenced this issue Dec 3, 2013

masover pushed a commit to masover/jruby that referenced this issue Dec 3, 2013

@masover

This comment has been minimized.

Copy link
Author

masover commented Dec 3, 2013

Added a PR: #1292.

I can add more thorough tests if needed, but this demonstrates the issue.

@enebo enebo closed this in #1292 Dec 4, 2013

enebo added a commit that referenced this issue Dec 4, 2013

enebo added a commit that referenced this issue Dec 4, 2013

subList size should be toIndex - fromIndex.
Related to #1274.
Tests pass now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.