RFR [8014066] Mistake in documentation of ArrayList#removeRange (original) (raw)

David Holmes david.holmes at oracle.com
Fri Mar 14 04:56:35 UTC 2014

Hi Ivan,

I think we need to apply the brakes here and back up a bit :)

First of all these aren't typo fixes they are spec changes and they will need to go through CCC for approval.

Second point is that the condition "fromIndex >= size()" is already captured by the condition "if {@code fromIndex} or {@code toIndex} is out of range". By definition fromIndex is out-of-range if it is < 0 or

= size(). So I don't see any error here even if there is some redundancy.

Third, a call a.removeRange(a.size(), a.size()) hits the normal dilemma of "what should be checked first?". The spec states that removeRange(n,n) is a no-op, so if we do that check first we just return, even for bad things like removeRange(-5, -5). Of course if we check all the preconditions first then we would in fact throw the IOOBE. I'm in the camp that says we check preconditions first because it detects silly mistakes sooner.

Fourth, your code change to add the additional pre-condition check:

   protected void removeRange(int fromIndex, int toIndex) {
       modCount++;

needs to be done before the change to modCount!

And finally, the AbstractList.removeRange change:

seems to reflect an implementation accident rather than a clear API design choice. If the toIndex is out of range then IndexOutOfBoundsException is what should be thrown. Otherwise you constrain any subtypes that override this to throw an exception type that only has meaning with the AbstractList implementation strategy - and by doing so you are making ArrayList.removeRange violate this new specification.

It is unfortunate that the existing specification(s) for removeRange are lacking this key detail on exception processing, and lack consistency between AbstractList and it's sublcass ArrayList. I think ArrayList has the better/cleaner specification and that should be pushed up AbstractList and AbstractList's implementation should be modified to explicitly check the pre-conditions.

David

On 14/03/2014 5:42 AM, Ivan Gerasimov wrote:

Sorry, I forgot to incorporate changes to AbstractList.removeRange() documentation, which you Martin had suggested.

Here's the webrev with those included: http://cr.openjdk.java.net/~igerasim/8014066/2/webrev/ Sincerely yours, Ivan

On 13.03.2014 23:03, Ivan Gerasimov wrote: Would you please take a look at the updated webrev: http://cr.openjdk.java.net/~igerasim/8014066/1/webrev/

In addition to the javadoc correction, it includes a regression test and a check for (fromIndex > toIndex). The last check turns out to be sufficient for removeRange() method to agree with the javadoc. Other checks are handled by the following System.arraycopy() call. Sincerely yours, Ivan

On 13.03.2014 22:27, Ivan Gerasimov wrote: On 13.03.2014 22:16, Ivan Gerasimov wrote:

On 13.03.2014 20:37, Martin Buchholz wrote: The corner case for removeRange(SIZE, SIZE) does seem rather tricky, and it's easy for an implementation to get it wrong. All the more reason to add tests!

It was a good idea to add a test for removeRange(). I just discovered that IOOB isn't thrown when you call removeRange(1, 0) or removeRange(4, 0). However, the exception is thrown when you call removeRange(5, 0). The fix seems to become a bit more than just a doc typo fix :-)

And when you do list.removeRange(1, 0), the list becomes longer. Sincerely yours, Ivan

On Thu, Mar 13, 2014 at 9:15 AM, Ivan Gerasimov <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> wrote: Thank you Chris! It is System.arraycopy() method, where checking is performed and the exception is thrown. Here's this code: if ( (((unsigned int) length + (unsigned int) srcpos) > (unsigned int) s->length()) || (((unsigned int) length + (unsigned int) dstpos) > (unsigned int) d->length()) ) { THROW(vmSymbols::javalangArrayIndexOutOfBoundsException()); } This confirms that size() is a valid value for fromIndex. Another way to think of it is that fromIndex <= toIndex, and toIndex can be equal to size(). Therefore, fromIndex can be equal to size() too. The documentation also says that 'If toIndex==fromIndex, this operation has no effect.', so removeRange(size(), size()) is a valid expression and should not cause an exception be thrown (and it actually does not). Sincerely yours, Ivan

On 13.03.2014 19:47, Chris Hegarty wrote: Ivan, This does look a little odd, but since fromIndex is inclusive I would think that it should throw if passed a value of size() ?? -Chris. On 13 Mar 2014, at 15:29, Ivan Gerasimov <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> wrote: Hello! Would you please review a simple fix of the javadoc for ArrayList#removeRange() method? The doc says that IndexOutOfBoundsException is thrown if fromIndex or toIndex is out of range (fromIndex < 0 ||_ _fromIndex >= size() || toIndex > size() || toIndex <_ _fromIndex)._ _The condition 'fromIndex >= size()' isn't true and should be removed from the doc. For example, the code list.removeRange(size(), size()) does not throw any exception. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8014066 WEBREV: http://cr.openjdk.java.net/~igerasim/8014066/0/webrev/ <http://cr.openjdk.java.net/%7Eigerasim/8014066/0/webrev/> Sincerely yours, Ivan

More information about the core-libs-dev mailing list