RFR [8014066] Mistake in documentation of ArrayList#removeRange

[Roger Riggs]

Hi,

Without getting into the details,  make sure that the behavior seen by
applications is not changed without careful and exhaustive review.
Usually, the specification is updated to match the existing long standing
behavior including exceptions and edge cases.

Roger




On 3/14/14 6:50 AM, Ivan Gerasimov wrote:
> Thanks David!
>
> On 14.03.2014 12:31, David Holmes wrote:
>>
>>>
>>>> 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.
>>>>
>>> I don't think that 'fromIndex < size()' restriction is quite natural.
>>
>> Seems perfectly natural to ensure the fromIndex is somewhere in the 
>> available range.
>>
>
> Sorry, what I meant was that it is too restrictive.
> 'fromIndex <= size()' is indeed natural.
>
>>
>>>> And finally, the AbstractList.removeRange change:
>>>>
>>>> +      * @throws NoSuchElementException if {@code (toIndex > size())}
>>>>
>>>> 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.
>>>>
>>> Again, I agree with you.
>>> One thing I noticed is that some methods I mentioned above
>>> (List.subList(), Arrays.sort(), etc) throw IllegalArgumentException 
>>> when
>>> fromIndex > toIndex, not IndexOutOfBoundException.
>>> Wouldn't it be more correct to adopt this into removeRange() too?
>>
>> Unfortunately there are a lot of inconsistencies in the API design 
>> throughout the JDK - even in classes that are somewhat related. That 
>> said List.subList throws IOOBE, But I do see examples of:
>>
>>     IllegalArgumentException - if fromIndex > toIndex
>>
>> which is definitely more correct if both values are in range, but 
>> then what if both are out of range?
>>
>> Anyway you can't just arbitrarily make changes to the specifications 
>> of these methods - even if trying to fix obvious inconsistencies. The 
>> main priority is that specs and implementations agree; and that specs 
>> in subtypes are consistent with the spec in the supertypes.
>>
>> I see a number of issues here that may be tricky to reconcile having 
>> regard for compatability.
>>
>
> What we have how is:
>
> AbstractList#removeRange()
> - spec misses throws section;
> - implementation has a bug: the function may throw 
> NoSuchElementException instead of IndexOutOfBoundsException;
>
> ArrayList#removeRange()
> - spec contains wrong condition 'fromIndex >= size()' ;
> - implementation has a bug: the condition fromIndex <= toIndex isn't 
> checked at all.
>
> Thus, we'll need to adjust both spec and implementation of both base 
> and derived classes.
> That's why I ask, if it may make sense to add IllegalArgumentException 
> for fromIndex > toIndex case, since the spec needs to be corrected 
> anyway.
> Of course, no problem to leave only IndexOutOfBoundsException.
>
> Concerning the ambiguity, which exception to throw if toIndex < 
> fromIndex and some of them is out of range.
> I believe, the conditions should be checked in the order they're 
> listed in the spec.
> Therefore, IndexOutOfBoundsException in this case.
>
> Sincerely yours,
> Ivan
>