Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 19.03.2014 23:37, schrieb Ulf Zibis: Am 19.03.2014 23:32, schrieb Martin Buchholz: No one is as performance obsessed as Ulf. :-D :-D :-D And additionally footprint obsessed -Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 19.03.2014 23:32, schrieb Martin Buchholz: No one is as performance obsessed as Ulf. :-D :-D :-D
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 19.03.2014 09:19, schrieb Ivan Gerasimov:
Thank you Ulf!
On 19.03.2014 2:12, Ulf Zibis wrote:
Am 18.03.2014 19:28, schrieb Ivan Gerasimov:
Assuming this last iteration is OK, should the next step be a CCC request?
Do you mean? :
/*
* ...
+ * It is assumed that fromIndex <= toIndex, otherwise the behaviour of this method is
undefined.
* ...
- * toIndex < fromIndex})
* ...
*/
protected void removeRange(int fromIndex, int toIndex) {
...
The (fromIndex > toIndex) condition is checked now, so the behavior of the method is defined - it
throws an exception.
Because Martin stated some days ago, that it normally should not occur, that removeRange is invoked
with toIndex < fromIndex, and checking this again unnecessarily decreases performance a little, I
was hoping, your "next CCC step" would be to drop this check.
-Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 19.03.2014 20:11, Martin Buchholz wrote: On Wed, Mar 19, 2014 at 1:19 AM, Ivan Gerasimov mailto:ivan.gerasi...@oracle.com>> wrote: Improving modCound handling can be done with a different RFE I guess, as it doesn't connected to the rest of the fix. I don't think modCount handling needs improvement. I didn't mean to propose modCount improvement. What I really meant was that modCount improvement (if required) should be separated from the removeRange's spec fix. If you're actually looking for follow-on improvements, I suggest globally replacing all of my Fun() poor-man emulations of lambdas with Real Lambdas. Thanks for suggestion, Martin. I crated an issue to track this: https://bugs.openjdk.java.net/browse/JDK-8037866 Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 19/03/14 08:29, David Holmes wrote: Fine by me. David On 19/03/2014 4:37 AM, Ivan Gerasimov wrote: Sorry, here's the link: http://cr.openjdk.java.net/~igerasim/8014066/4/webrev/ Looks fine to me too. -Chris. On 18.03.2014 22:28, Ivan Gerasimov wrote: Hello! Would you please take a look at the next iteration of webrev? I incorporated the last suggestions in it. When I first proposed a simple typo fix, I didn't think it's going to cause such a big discussion :) Assuming this last iteration is OK, should the next step be a CCC request? Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Fine by me. David On 19/03/2014 4:37 AM, Ivan Gerasimov wrote: Sorry, here's the link: http://cr.openjdk.java.net/~igerasim/8014066/4/webrev/ On 18.03.2014 22:28, Ivan Gerasimov wrote: Hello! Would you please take a look at the next iteration of webrev? I incorporated the last suggestions in it. When I first proposed a simple typo fix, I didn't think it's going to cause such a big discussion :) Assuming this last iteration is OK, should the next step be a CCC request? Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Thank you Ulf!
On 19.03.2014 2:12, Ulf Zibis wrote:
Am 18.03.2014 19:28, schrieb Ivan Gerasimov:
Assuming this last iteration is OK, should the next step be a CCC
request?
Do you mean? :
/*
* ...
+ * It is assumed that fromIndex <= toIndex, otherwise the
behaviour of this method is undefined.
* ...
- * toIndex < fromIndex})
* ...
*/
protected void removeRange(int fromIndex, int toIndex) {
...
The (fromIndex > toIndex) condition is checked now, so the behavior of
the method is defined - it throws an exception.
Please remove and replace inline by size - toIndex:
621 int numMoved = size - toIndex;
It is done this way throughout the class.
I don't think it has to be changed in this particular place.
Improving modCound handling can be done with a different RFE I guess, as
it doesn't connected to the rest of the fix.
Sincerely yours,
Ivan
About modCount:
Wouldn't it be more correct to code? :
616 if (fromIndex > toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 try {
621 modCount++;
622 System.arraycopy(elementData, toIndex, elementData,
fromIndex,
623 size - toIndex);
624 } catch (IndexOutOfBoundsException ioobe) {
625 modCount--;
626 throw ioobe;
627 }
Of even better :
000 private int[] modCount = { 0 };
...
616 if (fromIndex > toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 System.arraycopy(elementData, toIndex, elementData,
fromIndex,
621 size - toIndex, modCount);
Or :
000 public class ArrayList ... implements ModCounter {
001 private int modCount = 0;
001 void incModCount() {
001 modCount++;
004}
...
616 if (fromIndex > toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 System.arraycopy(elementData, toIndex, elementData,
fromIndex,
621 size - toIndex, this);
-Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 18.03.2014 23:17, schrieb Martin Buchholz: modCount is an imprecise concurrent modification mechanism. It doesn't have to be kept transactionally correct. Thanks, yes I know. But does it hurt to make it more precise? See this as a concept for a RFE. -Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Looks good. On Mar 18 2014, at 11:37 , Ivan Gerasimov wrote: > Sorry, here's the link: > http://cr.openjdk.java.net/~igerasim/8014066/4/webrev/ > > On 18.03.2014 22:28, Ivan Gerasimov wrote: >> Hello! >> >> Would you please take a look at the next iteration of webrev? >> I incorporated the last suggestions in it. >> >> When I first proposed a simple typo fix, I didn't think it's going to cause >> such a big discussion :) The most innocent questions have the most complicated answers. >> >> Assuming this last iteration is OK, should the next step be a CCC request? Yes. (I can be your reviewer for that, ping me once you have created the CCC) Mike
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 18.03.2014 19:28, schrieb Ivan Gerasimov:
Assuming this last iteration is OK, should the next step be a CCC request?
Do you mean? :
/*
* ...
+ * It is assumed that fromIndex <= toIndex, otherwise the behaviour of
this method is undefined.
* ...
- * toIndex < fromIndex})
* ...
*/
protected void removeRange(int fromIndex, int toIndex) {
...
Please remove and replace inline by size - toIndex:
621 int numMoved = size - toIndex;
About modCount:
Wouldn't it be more correct to code? :
616 if (fromIndex > toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 try {
621 modCount++;
622 System.arraycopy(elementData, toIndex, elementData, fromIndex,
623 size - toIndex);
624 } catch (IndexOutOfBoundsException ioobe) {
625 modCount--;
626 throw ioobe;
627 }
Of even better :
000 private int[] modCount = { 0 };
...
616 if (fromIndex > toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 System.arraycopy(elementData, toIndex, elementData, fromIndex,
621 size - toIndex, modCount);
Or :
000 public class ArrayList ... implements ModCounter {
001 private int modCount = 0;
001 void incModCount() {
001 modCount++;
004}
...
616 if (fromIndex > toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 System.arraycopy(elementData, toIndex, elementData, fromIndex,
621 size - toIndex, this);
-Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Sorry, here's the link: http://cr.openjdk.java.net/~igerasim/8014066/4/webrev/ On 18.03.2014 22:28, Ivan Gerasimov wrote: Hello! Would you please take a look at the next iteration of webrev? I incorporated the last suggestions in it. When I first proposed a simple typo fix, I didn't think it's going to cause such a big discussion :) Assuming this last iteration is OK, should the next step be a CCC request? Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Hello! Would you please take a look at the next iteration of webrev? I incorporated the last suggestions in it. When I first proposed a simple typo fix, I didn't think it's going to cause such a big discussion :) Assuming this last iteration is OK, should the next step be a CCC request? Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Hi Ivan, On 17/03/2014 8:37 AM, Ivan Gerasimov wrote: Here is yet another iteration of the fix: http://cr.openjdk.java.net/~igerasim/8014066/3/webrev/ 1) The condition 'fromIndex >= size()' is removed from the spec. I prefer removing it rather than replacing it with 'fromIndex > size()' for two reasons: - 'fromIndex > size()' already follows on from two other conditions (toIndex > size() || toIndex < fromIndex); - it is consistent with the spec for CopyOnWriteArrayList#removeRange(). Ok. 2) Kept the check for 'fromIndex > toIndex' in removeRange(). While I understand that this should not add anything significant to the current code, as currently removeRange() is always called with valid arguments. However, if it is stated in the spec that in case of 'fromIndex > toIndex' an exception is thrown, I believe it should be thrown, otherwise why it's stated? I postulate that there may have been some confusion here regarding the logical size of the ArrayList and the actual size of the elementData array. If size were actually elementData.length, then System.arraycopy would throw IOOBE if fromIndex > toIndex. So this extra check is fine, and fixes a bug, though to nitpick as it is spec'd as toIndexother way around? ;-) 3) Moved the test to MOAT.java The test looks a bit foreign over there, but reuses some of the infrastructure. I'll leave the testing comments to others more qualified. Thanks, David Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Hi Ivan, Just to see the effect of the change, I ran the test without the code change and it reported 331 failures and only 1 IndexOutOfBoundsException not thrown and that was in java.util.Collections$SingletonMap. (I can't appreciate the error reporting style with just a stack trace; especially given the generated cases in nested loops). I would have expected to see failures reporting the incorrect exception being thrown for ArrayList itself. Though most are likely due to the same condition outside the range, its hard to tell if there are other failures. Can you check that the test is detecting and reporting the right errors for the current implementation. Thanks, Roger On 3/16/2014 6:37 PM, Ivan Gerasimov wrote: Here is yet another iteration of the fix: http://cr.openjdk.java.net/~igerasim/8014066/3/webrev/ 1) The condition 'fromIndex >= size()' is removed from the spec. I prefer removing it rather than replacing it with 'fromIndex > size()' for two reasons: - 'fromIndex > size()' already follows on from two other conditions (toIndex > size() || toIndex < fromIndex); - it is consistent with the spec for CopyOnWriteArrayList#removeRange(). 2) Kept the check for 'fromIndex > toIndex' in removeRange(). While I understand that this should not add anything significant to the current code, as currently removeRange() is always called with valid arguments. However, if it is stated in the spec that in case of 'fromIndex > toIndex' an exception is thrown, I believe it should be thrown, otherwise why it's stated? 3) Moved the test to MOAT.java The test looks a bit foreign over there, but reuses some of the infrastructure. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 17.03.2014 2:52, Ulf Zibis wrote: Am 16.03.2014 23:37, schrieb Ivan Gerasimov: Here is yet another iteration of the fix: http://cr.openjdk.java.net/~igerasim/8014066/3/webrev/ 2) Kept the check for 'fromIndex > toIndex' in removeRange(). While I understand that this should not add anything significant to the current code, as currently removeRange() is always called with valid arguments. However, if it is stated in the spec that in case of 'fromIndex > toIndex' an exception is thrown, I believe it should be thrown, otherwise why it's stated? Isn't this a perfect situation to use an assert statement? Needless to say, then the spec must be adapted. Yeah, it might be more fair to write that it's assumed that the passed arguments are valid and use asserts as you suggest. Though I think that the current trend is to do as little modifications as possible. Sincerely yours, Ivan -Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 16.03.2014 23:37, schrieb Ivan Gerasimov: Here is yet another iteration of the fix: http://cr.openjdk.java.net/~igerasim/8014066/3/webrev/ 2) Kept the check for 'fromIndex > toIndex' in removeRange(). While I understand that this should not add anything significant to the current code, as currently removeRange() is always called with valid arguments. However, if it is stated in the spec that in case of 'fromIndex > toIndex' an exception is thrown, I believe it should be thrown, otherwise why it's stated? Isn't this a perfect situation to use an assert statement? Needless to say, then the spec must be adapted. -Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Here is yet another iteration of the fix: http://cr.openjdk.java.net/~igerasim/8014066/3/webrev/ 1) The condition 'fromIndex >= size()' is removed from the spec. I prefer removing it rather than replacing it with 'fromIndex > size()' for two reasons: - 'fromIndex > size()' already follows on from two other conditions (toIndex > size() || toIndex < fromIndex); - it is consistent with the spec for CopyOnWriteArrayList#removeRange(). 2) Kept the check for 'fromIndex > toIndex' in removeRange(). While I understand that this should not add anything significant to the current code, as currently removeRange() is always called with valid arguments. However, if it is stated in the spec that in case of 'fromIndex > toIndex' an exception is thrown, I believe it should be thrown, otherwise why it's stated? 3) Moved the test to MOAT.java The test looks a bit foreign over there, but reuses some of the infrastructure. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Hi Martin, On 15/03/2014 11:24 PM, Martin Buchholz wrote: We understand the intent of removeRange better now. I agree that making further improvements is tricky. Almost impossible I'd say :) I think we can all agree that the initial patch http://cr.openjdk.java.net/~igerasim/8014066/0/webrev/jdk.patch is a clear improvement (simple spec bug fix) fromIndex == size must be OK because it results from a call to clear on an already empty list. Maybe we should check that unambitious fix in first. I agree. Though I'd suggest that in keeping with the "fixing a typo" theme we simply change: * fromIndex >= size() || to * fromIndex > size() || Although this constraint is already implicit in the "in range" requirement, a one character change is much easier to accept as correct. David - Changing the spec for removeRange to add more requirements on subclass implementations seems wrong because there are existing implementations and because the (underspecified) contract is that removeRange will only be called from a call to clear, which in turn will ensure that the indices are legal. Adding more bounds checks now will just add overhead without catching user mistakes in practice. Improving the spec to clarify the contract as originally designed by Josh would be good, but it will be hard to find good wording and get approval from all the interested parties. Software is hard. On Fri, Mar 14, 2014 at 4:42 AM, Doug Lea wrote: On 03/14/2014 02:38 AM, Martin Buchholz wrote: On Thu, Mar 13, 2014 at 3:59 PM, Doug Lea mailto:d...@cs.oswego.edu>> wrote: On 03/13/2014 12:34 PM, Martin Buchholz wrote: I notice there are zero jtreg tests for removeRange. That should be fixed. I notice there is a removeRange in CopyOnWriteArrayList, but it is package-private instead of "protected", which seems like an oversight. Can Doug remember any history on that? CopyOnWriteArrayList does not extend AbstractList, but its sublist does. The sublist relies on COWAL.removeRange only for clear. So COWAL sublist clearing uses the same idea as AbstractList, and gives it the same name, but it is not the same AbstractList method, so need not be protected. Ahh OK, I think the party line for *users* is if they want to remove a range of elements from a list, use list.subList(fromIndex, toIndex).clear (), so there's no advantage in making COWAL.removeRange a public interface. Right. This relates to the question of range checks for removeRange. Josh Block created removeRange as part of an incompletely documented recipe for AbstractList-based List implementations. It was intended that people creating new kinds of Lists implement this as a helper method to simplify sublist implementations. It is designed to be called only from other public methods that would either themselves perform range checks or skip them if statically unnecessary. So, there aren't any redundant checks in the default removeRange. Leaving this partially exposed as "protected" doesn't quite ensure that implementors follow this guidance, but in practice, I suspect that they all have. So it is not clear whether changing the spec or the implementation would be doing anyone a favor. -Doug
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 14/03/2014 10:22 PM, Doug Lea wrote: On 03/14/2014 08:07 AM, David Holmes wrote: So what you are saying is that protected overrides of protected methods are not required to honor the specification of the super method? Not always, but ... That certainly gives some implementation flexibility, but I don't think I've ever seen it expressed as a rule. ... this is the convention for @implSpecs for default methods on interfaces. Which probably would have been used here instead of AbstractList had they existed back in JDK1.2. However @throws is not part of implSpecs. AbstractList should have defined the potential exceptions and subclass implementations would have to conform. David -Doug
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Thank you Martin! On 15.03.2014 17:24, Martin Buchholz wrote: We understand the intent of removeRange better now. I agree that making further improvements is tricky. I think we can all agree that the initial patch http://cr.openjdk.java.net/~igerasim/8014066/0/webrev/jdk.patch is a clear improvement (simple spec bug fix) fromIndex == size must be OK because it results from a call to clear on an already empty list. Maybe we should check that unambitious fix in first. What about a regtest then? Wouldn't it be good to check at least that removeRange(size(), size()) works, and removeRange(-1,size()) or (0, size()+1) throw the exception? Sincerely yours, Ivan Changing the spec for removeRange to add more requirements on subclass implementations seems wrong because there are existing implementations and because the (underspecified) contract is that removeRange will only be called from a call to clear, which in turn will ensure that the indices are legal. Adding more bounds checks now will just add overhead without catching user mistakes in practice. Improving the spec to clarify the contract as originally designed by Josh would be good, but it will be hard to find good wording and get approval from all the interested parties. Software is hard. On Fri, Mar 14, 2014 at 4:42 AM, Doug Lea wrote: On 03/14/2014 02:38 AM, Martin Buchholz wrote: On Thu, Mar 13, 2014 at 3:59 PM, Doug Lea mailto:d...@cs.oswego.edu>> wrote: On 03/13/2014 12:34 PM, Martin Buchholz wrote: I notice there are zero jtreg tests for removeRange. That should be fixed. I notice there is a removeRange in CopyOnWriteArrayList, but it is package-private instead of "protected", which seems like an oversight. Can Doug remember any history on that? CopyOnWriteArrayList does not extend AbstractList, but its sublist does. The sublist relies on COWAL.removeRange only for clear. So COWAL sublist clearing uses the same idea as AbstractList, and gives it the same name, but it is not the same AbstractList method, so need not be protected. Ahh OK, I think the party line for *users* is if they want to remove a range of elements from a list, use list.subList(fromIndex, toIndex).clear (), so there's no advantage in making COWAL.removeRange a public interface. Right. This relates to the question of range checks for removeRange. Josh Block created removeRange as part of an incompletely documented recipe for AbstractList-based List implementations. It was intended that people creating new kinds of Lists implement this as a helper method to simplify sublist implementations. It is designed to be called only from other public methods that would either themselves perform range checks or skip them if statically unnecessary. So, there aren't any redundant checks in the default removeRange. Leaving this partially exposed as "protected" doesn't quite ensure that implementors follow this guidance, but in practice, I suspect that they all have. So it is not clear whether changing the spec or the implementation would be doing anyone a favor. -Doug
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On Mar 14 2014, at 05:14 , Ivan Gerasimov wrote: > > On 14.03.2014 16:02, David Holmes wrote: >> Ivan, >> >> On 14/03/2014 9:19 PM, Ivan Gerasimov wrote: >>> Thanks Peter for the comments. >>> >>> On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: > 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... >>> >>> In my opinion, the order of the checks should match the order of the >>> listed exceptions in the spec. The @throws tags aren't ordered. The javadoc tool is free to re-order them (such as alphabetization or grouping by super class). Specifying the exception behaviour to this degree seems like vast overspecification. What's the value other than to someone writing a validation test who might have to catch or expect both exceptions? In real user code it doesn't seem important which particular exception is thrown. In either case the fix that must be applied by the programmer is likely the same in either case. Mike
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 14.03.2014 18:18, Peter Levart wrote: On 03/14/2014 01:14 PM, Ivan Gerasimov wrote: On 14.03.2014 16:02, David Holmes wrote: Ivan, On 14/03/2014 9:19 PM, Ivan Gerasimov wrote: Thanks Peter for the comments. On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... In my opinion, the order of the checks should match the order of the listed exceptions in the spec. That's a nice proposal but unfortunately there has never been a rule that you have to document exceptions in the order you expect them to be checked; nor do you have to implement exception checks in the order they are documented. So it is too late to try and enforce this now. Sigh. The order can still be stated explicitly in the doc, if it's decided to throw IllegalArgumentException. Or the checks could be documented in a way that they have disjunctive conditions so that the order doesn't matter: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) and neither index is negative nor greater than size() Yes, this is the most clear way! Sincerely yours, Ivan Regards, Peter And, of course, there's no such problem if IndexOutOfBoundException is the only exception thrown. Sincerely yours, Ivan David - If the order is: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) then removeRange(0, -1) should throw IndexOutOfBoundException. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 03/14/2014 01:14 PM, Ivan Gerasimov wrote: On 14.03.2014 16:02, David Holmes wrote: Ivan, On 14/03/2014 9:19 PM, Ivan Gerasimov wrote: Thanks Peter for the comments. On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... In my opinion, the order of the checks should match the order of the listed exceptions in the spec. That's a nice proposal but unfortunately there has never been a rule that you have to document exceptions in the order you expect them to be checked; nor do you have to implement exception checks in the order they are documented. So it is too late to try and enforce this now. Sigh. The order can still be stated explicitly in the doc, if it's decided to throw IllegalArgumentException. Or the checks could be documented in a way that they have disjunctive conditions so that the order doesn't matter: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) and neither index is negative nor greater than size() Regards, Peter And, of course, there's no such problem if IndexOutOfBoundException is the only exception thrown. Sincerely yours, Ivan David - If the order is: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) then removeRange(0, -1) should throw IndexOutOfBoundException. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 03/14/2014 08:07 AM, David Holmes wrote: So what you are saying is that protected overrides of protected methods are not required to honor the specification of the super method? Not always, but ... That certainly gives some implementation flexibility, but I don't think I've ever seen it expressed as a rule. ... this is the convention for @implSpecs for default methods on interfaces. Which probably would have been used here instead of AbstractList had they existed back in JDK1.2. -Doug
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 14/03/2014 10:14 PM, Ivan Gerasimov wrote: On 14.03.2014 16:02, David Holmes wrote: Ivan, On 14/03/2014 9:19 PM, Ivan Gerasimov wrote: Thanks Peter for the comments. On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... In my opinion, the order of the checks should match the order of the listed exceptions in the spec. That's a nice proposal but unfortunately there has never been a rule that you have to document exceptions in the order you expect them to be checked; nor do you have to implement exception checks in the order they are documented. So it is too late to try and enforce this now. Sigh. The order can still be stated explicitly in the doc, if it's decided to throw IllegalArgumentException. Of course the order can be explicitly stated. My point was that you can't just look at existing spec and decide that defines the order of checking. Cheers, David And, of course, there's no such problem if IndexOutOfBoundException is the only exception thrown. Sincerely yours, Ivan David - If the order is: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) then removeRange(0, -1) should throw IndexOutOfBoundException. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 14.03.2014 16:02, David Holmes wrote: Ivan, On 14/03/2014 9:19 PM, Ivan Gerasimov wrote: Thanks Peter for the comments. On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... In my opinion, the order of the checks should match the order of the listed exceptions in the spec. That's a nice proposal but unfortunately there has never been a rule that you have to document exceptions in the order you expect them to be checked; nor do you have to implement exception checks in the order they are documented. So it is too late to try and enforce this now. Sigh. The order can still be stated explicitly in the doc, if it's decided to throw IllegalArgumentException. And, of course, there's no such problem if IndexOutOfBoundException is the only exception thrown. Sincerely yours, Ivan David - If the order is: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) then removeRange(0, -1) should throw IndexOutOfBoundException. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Hi Doug, On 14/03/2014 9:42 PM, Doug Lea wrote: On 03/14/2014 02:38 AM, Martin Buchholz wrote: On Thu, Mar 13, 2014 at 3:59 PM, Doug Lea mailto:d...@cs.oswego.edu>> wrote: On 03/13/2014 12:34 PM, Martin Buchholz wrote: I notice there are zero jtreg tests for removeRange. That should be fixed. I notice there is a removeRange in CopyOnWriteArrayList, but it is package-private instead of "protected", which seems like an oversight. Can Doug remember any history on that? CopyOnWriteArrayList does not extend AbstractList, but its sublist does. The sublist relies on COWAL.removeRange only for clear. So COWAL sublist clearing uses the same idea as AbstractList, and gives it the same name, but it is not the same AbstractList method, so need not be protected. Ahh OK, I think the party line for *users* is if they want to remove a range of elements from a list, use list.subList(fromIndex, toIndex).clear (), so there's no advantage in making COWAL.removeRange a public interface. Right. This relates to the question of range checks for removeRange. Josh Block created removeRange as part of an incompletely documented recipe for AbstractList-based List implementations. It was intended that people creating new kinds of Lists implement this as a helper method to simplify sublist implementations. It is designed to be called only from other public methods that would either themselves perform range checks or skip them if statically unnecessary. So, there aren't any redundant checks in the default removeRange. Leaving this partially exposed as "protected" doesn't quite ensure that implementors follow this guidance, but in practice, I suspect that they all have. So it is not clear whether changing the spec or the implementation would be doing anyone a favor. So what you are saying is that protected overrides of protected methods are not required to honor the specification of the super method? That certainly gives some implementation flexibility, but I don't think I've ever seen it expressed as a rule. David - -Doug
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Ivan, On 14/03/2014 9:19 PM, Ivan Gerasimov wrote: Thanks Peter for the comments. On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... In my opinion, the order of the checks should match the order of the listed exceptions in the spec. That's a nice proposal but unfortunately there has never been a rule that you have to document exceptions in the order you expect them to be checked; nor do you have to implement exception checks in the order they are documented. So it is too late to try and enforce this now. David - If the order is: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) then removeRange(0, -1) should throw IndexOutOfBoundException. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 03/14/2014 02:38 AM, Martin Buchholz wrote: On Thu, Mar 13, 2014 at 3:59 PM, Doug Lea mailto:d...@cs.oswego.edu>> wrote: On 03/13/2014 12:34 PM, Martin Buchholz wrote: I notice there are zero jtreg tests for removeRange. That should be fixed. I notice there is a removeRange in CopyOnWriteArrayList, but it is package-private instead of "protected", which seems like an oversight. Can Doug remember any history on that? CopyOnWriteArrayList does not extend AbstractList, but its sublist does. The sublist relies on COWAL.removeRange only for clear. So COWAL sublist clearing uses the same idea as AbstractList, and gives it the same name, but it is not the same AbstractList method, so need not be protected. Ahh OK, I think the party line for *users* is if they want to remove a range of elements from a list, use list.subList(fromIndex, toIndex).clear (), so there's no advantage in making COWAL.removeRange a public interface. Right. This relates to the question of range checks for removeRange. Josh Block created removeRange as part of an incompletely documented recipe for AbstractList-based List implementations. It was intended that people creating new kinds of Lists implement this as a helper method to simplify sublist implementations. It is designed to be called only from other public methods that would either themselves perform range checks or skip them if statically unnecessary. So, there aren't any redundant checks in the default removeRange. Leaving this partially exposed as "protected" doesn't quite ensure that implementors follow this guidance, but in practice, I suspect that they all have. So it is not clear whether changing the spec or the implementation would be doing anyone a favor. -Doug
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Thanks Peter for the comments. On 14.03.2014 14:53, Peter Levart wrote: On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... In my opinion, the order of the checks should match the order of the listed exceptions in the spec. If the order is: @throws IndexOutOfBoundException if either index is negative or either index is greater than size() @throws IllegalArgumentException if (fromIndex > toIndex) then removeRange(0, -1) should throw IndexOutOfBoundException. Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 03/14/2014 08:05 AM, Ivan Gerasimov wrote: 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? The question is, what exception should be thrown for removeRange(0, -1) then... The order of checks matters and should be specified if two kinds of exceptions are thrown... Regards, Peter
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Hi Ivan,
On 14/03/2014 5:05 PM, Ivan Gerasimov wrote:
Thank you David for your reply!
On 14.03.2014 8:56, David Holmes wrote:
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.
Yes, sure. I haven't done that before, so will need to find someone to
assist me for the first time.
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.
I've scanned through several other methods in JDK, where the pair
'fromIndex, toIndex' is used as the arguments.
For example, List.subList(), BitSet.clear(), BitSet.flip(),
BitSet.get(), BitSet.set(), Arrays.sort(), Arrays.binarySearch(),
Arrays.fill() and several others.
None of these methods have this kind of restriction, at least in the
documentation.
List.subList states:
IndexOutOfBoundsException - for an illegal endpoint index value
(fromIndex < 0 || toIndex > size || fromIndex > toIndex)
which means that fromIndex <= toIndex <= size; hence fromIndex > size()
is illegal.
Also CharSequence.subSequence:
IndexOutOfBoundsException - if start or end are negative, if end is
greater than length(), or if start is greater than end
As I said fromIndex >= size() is actually captured by saying fromIndex
must be in range.
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.
In my opinion a.removeRange(a.size(), a.size()) is a valid expression,
while removeRange(-5, -5) is not.
For example, if had a need to remove some elements up to the end of the
list, I would have toIndex fixed to 'a.end()': a.remove(fromIndex,
a.end()).
In this situation, I would need to have a way to specify an empty set of
items to be removed, thus having fromIndex == a.end().
That's a good point. I think the prohibition on fromIndex==size() is
wrong, as per the other examples (subList, subSequence).
I agree that preconditions should be checked first, so removeRange(-5,
-5) should throw an exception.
Fourth, your code change to add the additional pre-condition check:
protected void removeRange(int fromIndex, int toIndex) {
modCount++;
+ if (fromIndex > toIndex)
+ throw new IndexOutOfBoundsException(
needs to be done _before_ the change to modCount!
I placed it right before a call to System.copyarray(), where all other
checks are performed.
That is a bug in the existing code then. If a precondition fails then
the data structure is not modified and the modCount should not be
changed. This is what happens when you piggy-back pre-condition checks.
But I don't mind moving it to the top of the method.
Thanks - every little helps :)
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
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Thank you David for your reply!
On 14.03.2014 8:56, David Holmes wrote:
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.
Yes, sure. I haven't done that before, so will need to find someone to
assist me for the first time.
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.
I've scanned through several other methods in JDK, where the pair
'fromIndex, toIndex' is used as the arguments.
For example, List.subList(), BitSet.clear(), BitSet.flip(),
BitSet.get(), BitSet.set(), Arrays.sort(), Arrays.binarySearch(),
Arrays.fill() and several others.
None of these methods have this kind of restriction, at least in the
documentation.
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.
In my opinion a.removeRange(a.size(), a.size()) is a valid expression,
while removeRange(-5, -5) is not.
For example, if had a need to remove some elements up to the end of the
list, I would have toIndex fixed to 'a.end()': a.remove(fromIndex, a.end()).
In this situation, I would need to have a way to specify an empty set of
items to be removed, thus having fromIndex == a.end().
I agree that preconditions should be checked first, so removeRange(-5,
-5) should throw an exception.
Fourth, your code change to add the additional pre-condition check:
protected void removeRange(int fromIndex, int toIndex) {
modCount++;
+ if (fromIndex > toIndex)
+ throw new IndexOutOfBoundsException(
needs to be done _before_ the change to modCount!
I placed it right before a call to System.copyarray(), where all other
checks are performed.
But I don't mind moving it to the top of the method.
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?
Sincerely yours,
Ivan
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 do
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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++;
+ if (fromIndex > toIndex)
+ throw new IndexOutOfBoundsException(
needs to be done _before_ the change to modCount!
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.
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
mailto:ivan.gerasi...@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) src_pos) >
(unsigned int) s->length())
|| (((unsigned int) length + (unsigned int) dst_pos) >
(unsigned int) d->length()) ) {
THROW(vmSymbols::java_lang_ArrayIndexOutOfBoundsException());
}
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
mailto:ivan.gerasi...@oracle.com>>
wrote:
Hello!
Would you please review a simple fix of the javadoc for
ArrayList#removeRange() method?
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 03/13/2014 12:34 PM, Martin Buchholz wrote:
I notice there are zero jtreg tests for removeRange. That should be fixed.
I notice there is a removeRange in CopyOnWriteArrayList, but it is
package-private instead of "protected", which seems like an oversight. Can Doug
remember any history on that?
CopyOnWriteArrayList does not extend AbstractList, but its
sublist does. The sublist relies on COWAL.removeRange only for clear.
So COWAL sublist clearing uses the same idea as
AbstractList, and gives it the same name, but it is not the
same AbstractList method, so need not be protected.
-Doug
I notice that AbstractList.removeRange contains no @throws. That should be
fixed?
The existing @throws javadoc from CopyOnWriteArrayList seems better:
* @throws IndexOutOfBoundsException if fromIndex or toIndex out of range
* ({@code fromIndex < 0 || toIndex > size() || toIndex <
fromIndex})
This paragraph in AbstractList
* This method is called by the {@code clear} operation on this list
* and its subLists. Overriding this method to take advantage of
* the internals of the list implementation can substantially
* improve the performance of the {@code clear} operation on this list
* and its subLists.
looks like it belongs inside the @implSpec (it's not a requirement on
subclasses)
ObTesting (a start)
import java.util.*;
public class RemoveRange {
static class PublicArrayList extends ArrayList {
PublicArrayList(int cap) { super(cap); }
public void removeRange(int fromIndex, int toIndex) {
super.removeRange(fromIndex, toIndex);
}
}
public static void main(String[] args) throws Throwable {
PublicArrayList x = new PublicArrayList(11);
for (int i = 0; i < 11; i++) x.add(null);
x.removeRange(x.size(), x.size());
}
}
On Thu, Mar 13, 2014 at 8:29 AM, Ivan Gerasimov mailto:ivan.gerasi...@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/
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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
mailto:ivan.gerasi...@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) src_pos) >
(unsigned int) s->length())
|| (((unsigned int) length + (unsigned int) dst_pos) >
(unsigned int) d->length()) ) {
THROW(vmSymbols::java_lang_ArrayIndexOutOfBoundsException());
}
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
mailto:ivan.gerasi...@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/
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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
mailto:ivan.gerasi...@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) src_pos) >
(unsigned int) s->length())
|| (((unsigned int) length + (unsigned int) dst_pos) >
(unsigned int) d->length()) ) {
THROW(vmSymbols::java_lang_ArrayIndexOutOfBoundsException());
}
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
mailto:ivan.gerasi...@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/
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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
mailto:ivan.gerasi...@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) src_pos) >
(unsigned int) s->length())
|| (((unsigned int) length + (unsigned int) dst_pos) >
(unsigned int) d->length()) ) {
THROW(vmSymbols::java_lang_ArrayIndexOutOfBoundsException());
}
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
mailto:ivan.gerasi...@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/
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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 :-)
Sincerely yours,
Ivan
On Thu, Mar 13, 2014 at 9:15 AM, Ivan Gerasimov
mailto:ivan.gerasi...@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) src_pos) >
(unsigned int) s->length())
|| (((unsigned int) length + (unsigned int) dst_pos) >
(unsigned int) d->length()) ) {
THROW(vmSymbols::java_lang_ArrayIndexOutOfBoundsException());
}
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
mailto:ivan.gerasi...@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/
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Thank you Martin for your feedback!
I'll add the test with the next webrev.
Here's the test I used to check that the exception isn't thrown:
class Test extends java.util.ArrayList {
public static void main(String[] args) throws Throwable {
Test list = new Test();
list.add(1);
list.removeRange(list.size(), list.size());
}
}
Sincerely yours,
Ivan
On 13.03.2014 20:34, Martin Buchholz wrote:
I notice there are zero jtreg tests for removeRange. That should be
fixed.
I notice there is a removeRange in CopyOnWriteArrayList, but it is
package-private instead of "protected", which seems like an oversight.
Can Doug remember any history on that?
I notice that AbstractList.removeRange contains no @throws. That
should be fixed?
The existing @throws javadoc from CopyOnWriteArrayList seems better:
* @throws IndexOutOfBoundsException if fromIndex or toIndex out
of range
* ({@code fromIndex < 0 || toIndex > size() || toIndex <
fromIndex})
This paragraph in AbstractList
* This method is called by the {@code clear} operation on this
list
* and its subLists. Overriding this method to take advantage of
* the internals of the list implementation can substantially
* improve the performance of the {@code clear} operation on this list
* and its subLists.
looks like it belongs inside the @implSpec (it's not a requirement on
subclasses)
ObTesting (a start)
import java.util.*;
public class RemoveRange {
static class PublicArrayList extends ArrayList {
PublicArrayList(int cap) { super(cap); }
public void removeRange(int fromIndex, int toIndex) {
super.removeRange(fromIndex, toIndex);
}
}
public static void main(String[] args) throws Throwable {
PublicArrayList x = new PublicArrayList(11);
for (int i = 0; i < 11; i++) x.add(null);
x.removeRange(x.size(), x.size());
}
}
On Thu, Mar 13, 2014 at 8:29 AM, Ivan Gerasimov
mailto:ivan.gerasi...@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/
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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) src_pos) > (unsigned
int) s->length())
|| (((unsigned int) length + (unsigned int) dst_pos) > (unsigned
int) d->length()) ) {
THROW(vmSymbols::java_lang_ArrayIndexOutOfBoundsException());
}
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 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/
Sincerely yours,
Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
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 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/ > > Sincerely yours, > Ivan
RFR [8014066] Mistake in documentation of ArrayList#removeRange
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/ Sincerely yours, Ivan
