RFR 8218228 : The constructor StringBuffer(CharSequence) violates spec for negatively sized argument
Hello! The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """ However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length). It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity). Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 -- With kind regards, Ivan Gerasimov
Hi Ivan, On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
Hello!
The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """
However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length).
How can you have a negative length CharSequence ?? If its an empty CharSequence then it should return the empty buffer as indicated. Cheers, David
It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity).
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
Hi David! On 2/7/19 5:16 PM, David Holmes wrote:
Hi Ivan,
On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
Hello!
The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """
However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length).
How can you have a negative length CharSequence ??
A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though. That's why I propose just removing the mentioning negative length, and not changing the behavior. The proposed code change is to only unify the behavior for any negative value of length.
If its an empty CharSequence then it should return the empty buffer as indicated.
Empty CharSequence is processed correctly already. With kind regards, Ivan
Cheers, David
It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity).
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
-- With kind regards, Ivan Gerasimov
On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
Hi David!
On 2/7/19 5:16 PM, David Holmes wrote:
Hi Ivan,
On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
Hello!
The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """
However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length).
How can you have a negative length CharSequence ??
A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
That's why I propose just removing the mentioning negative length, and not changing the behavior.
The proposed code change is to only unify the behavior for any negative value of length.
Ok.
If its an empty CharSequence then it should return the empty buffer as indicated.
Empty CharSequence is processed correctly already.
Okay so by removing this part: - * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned. you're relying on the main specification to implicitly handle the empty case. David -----
With kind regards, Ivan
Cheers, David
It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity).
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
On 2/7/19 6:33 PM, David Holmes wrote:
On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
Hi David!
On 2/7/19 5:16 PM, David Holmes wrote:
Hi Ivan,
On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
Hello!
The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """
However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length).
How can you have a negative length CharSequence ??
A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence. (To be precise, there is one candidate, but I'll file a separate bug to fix it.)
That's why I propose just removing the mentioning negative length, and not changing the behavior.
The proposed code change is to only unify the behavior for any negative value of length.
Ok.
If its an empty CharSequence then it should return the empty buffer as indicated.
Empty CharSequence is processed correctly already.
Okay so by removing this part:
- * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned.
you're relying on the main specification to implicitly handle the empty case.
Yes. With kind regards, Ivan
David -----
With kind regards, Ivan
Cheers, David
It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity).
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
-- With kind regards, Ivan Gerasimov
Hi Ivan, In the direction of not changing the behavior; the webrev does change the behavior. In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException. AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents. The new Test should pass both before and after the change to the code. Roger On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
On 2/7/19 6:33 PM, David Holmes wrote:
On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
Hi David!
On 2/7/19 5:16 PM, David Holmes wrote:
Hi Ivan,
On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
Hello!
The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """
However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length).
How can you have a negative length CharSequence ??
A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence.
(To be precise, there is one candidate, but I'll file a separate bug to fix it.)
That's why I propose just removing the mentioning negative length, and not changing the behavior.
The proposed code change is to only unify the behavior for any negative value of length.
Ok.
If its an empty CharSequence then it should return the empty buffer as indicated.
Empty CharSequence is processed correctly already.
Okay so by removing this part:
- * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned.
you're relying on the main specification to implicitly handle the empty case.
Yes.
With kind regards, Ivan
David -----
With kind regards, Ivan
Cheers, David
It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity).
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
Hi Roger! That's because two tiny changes are combined in the patch: 1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior. 2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length \in [-16, -1]. I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together. With kind regards, Ivan On 2/8/19 1:49 PM, Roger Riggs wrote:
Hi Ivan,
In the direction of not changing the behavior; the webrev does change the behavior.
In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException.
AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents.
The new Test should pass both before and after the change to the code.
Roger
On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
On 2/7/19 6:33 PM, David Holmes wrote:
On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
Hi David!
On 2/7/19 5:16 PM, David Holmes wrote:
Hi Ivan,
On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
Hello!
The specification states: """ If the length of the specified CharSequence is less than or equal to zero, then an empty buffer of capacity 16 is returned. """
However, the current implementation throws either NegativeArraySizeException or IndexOutOfBounds instead (the actual exception type depends on the value of reported negative length).
How can you have a negative length CharSequence ??
A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence.
(To be precise, there is one candidate, but I'll file a separate bug to fix it.)
That's why I propose just removing the mentioning negative length, and not changing the behavior.
The proposed code change is to only unify the behavior for any negative value of length.
Ok.
If its an empty CharSequence then it should return the empty buffer as indicated.
Empty CharSequence is processed correctly already.
Okay so by removing this part:
- * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned.
you're relying on the main specification to implicitly handle the empty case.
Yes.
With kind regards, Ivan
David -----
With kind regards, Ivan
Cheers, David
It is proposed to do two things: 1) remove the problematic sentence from the javadoc (CSR is filed). 2) unify the exception type thrown, if the argument reports negative length. NegativeArraySizeException will be consistent with, for example, StringBuffer(negativeCapacity).
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
-- With kind regards, Ivan Gerasimov
Hi Ivan, I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior. I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated. Thanks, Roger On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
Hi Roger!
That's because two tiny changes are combined in the patch:
1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior.
2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length in [-16, -1].
I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together.
With kind regards,
Ivan
On 2/8/19 1:49 PM, Roger Riggs wrote:
Hi Ivan,
In the direction of not changing the behavior; the webrev does change the behavior.
In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException.
AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents.
The new Test should pass both before and after the change to the code.
Roger
On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
On 2/7/19 6:33 PM, David Holmes wrote:
On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
Hi David!
On 2/7/19 5:16 PM, David Holmes wrote:
Hi Ivan,
On 8/02/2019 11:02 am, Ivan Gerasimov wrote: > Hello! > > The specification states: > """ > If the length of the specified CharSequence is less than or > equal to zero, then an empty buffer of capacity 16 is returned. > """ > > However, the current implementation throws either > NegativeArraySizeException or IndexOutOfBounds instead (the > actual exception type depends on the value of reported negative > length).
How can you have a negative length CharSequence ??
A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence.
(To be precise, there is one candidate, but I'll file a separate bug to fix it.)
That's why I propose just removing the mentioning negative length, and not changing the behavior.
The proposed code change is to only unify the behavior for any negative value of length.
Ok.
If its an empty CharSequence then it should return the empty buffer as indicated.
Empty CharSequence is processed correctly already.
Okay so by removing this part:
- * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned.
you're relying on the main specification to implicitly handle the empty case.
Yes.
With kind regards, Ivan
David -----
With kind regards, Ivan
Cheers, David
> It is proposed to do two things: > 1) remove the problematic sentence from the javadoc (CSR is filed). > 2) unify the exception type thrown, if the argument reports > negative length. > NegativeArraySizeException will be consistent with, for example, > StringBuffer(negativeCapacity). > > Would you please help review the fix? > > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 > WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ > CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >
Hi Roger! On 2/11/19 7:30 AM, Roger Riggs wrote:
Hi Ivan,
I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior.
I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated.
Yes, this makes sense. I've updated CSR accordingly, and added a comment to the bug to make the intention of the fix clear. Could you please review the updated CSR? With kind regards, Ivan
Thanks, Roger
On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
Hi Roger!
That's because two tiny changes are combined in the patch:
1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior.
2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length in [-16, -1].
I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together.
With kind regards,
Ivan
On 2/8/19 1:49 PM, Roger Riggs wrote:
Hi Ivan,
In the direction of not changing the behavior; the webrev does change the behavior.
In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException.
AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents.
The new Test should pass both before and after the change to the code.
Roger
On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
On 2/7/19 6:33 PM, David Holmes wrote:
On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
Hi David!
On 2/7/19 5:16 PM, David Holmes wrote: > Hi Ivan, > > On 8/02/2019 11:02 am, Ivan Gerasimov wrote: >> Hello! >> >> The specification states: >> """ >> If the length of the specified CharSequence is less than or >> equal to zero, then an empty buffer of capacity 16 is returned. >> """ >> >> However, the current implementation throws either >> NegativeArraySizeException or IndexOutOfBounds instead (the >> actual exception type depends on the value of reported negative >> length). > > How can you have a negative length CharSequence ?? > A custom CharSequence returning negative length() can be created. Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence.
(To be precise, there is one candidate, but I'll file a separate bug to fix it.)
That's why I propose just removing the mentioning negative length, and not changing the behavior.
The proposed code change is to only unify the behavior for any negative value of length.
Ok.
> If its an empty CharSequence then it should return the empty > buffer as indicated. > Empty CharSequence is processed correctly already.
Okay so by removing this part:
- * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned.
you're relying on the main specification to implicitly handle the empty case.
Yes.
With kind regards, Ivan
David -----
With kind regards, Ivan
> Cheers, > David > >> It is proposed to do two things: >> 1) remove the problematic sentence from the javadoc (CSR is >> filed). >> 2) unify the exception type thrown, if the argument reports >> negative length. >> NegativeArraySizeException will be consistent with, for >> example, StringBuffer(negativeCapacity). >> >> Would you please help review the fix? >> >> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 >> WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ >> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >> >
-- With kind regards, Ivan Gerasimov
Hi Ivan, The change to consistently throw NegativeSizeArrayException applies to StringBuilder also. Please mention that with the solution. Thanks, Roger On 02/11/2019 06:12 PM, Ivan Gerasimov wrote:
Hi Roger!
On 2/11/19 7:30 AM, Roger Riggs wrote:
Hi Ivan,
I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior.
I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated.
Yes, this makes sense. I've updated CSR accordingly, and added a comment to the bug to make the intention of the fix clear.
Could you please review the updated CSR?
With kind regards, Ivan
Thanks, Roger
On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
Hi Roger!
That's because two tiny changes are combined in the patch:
1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior.
2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length in [-16, -1].
I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together.
With kind regards,
Ivan
On 2/8/19 1:49 PM, Roger Riggs wrote:
Hi Ivan,
In the direction of not changing the behavior; the webrev does change the behavior.
In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException.
AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents.
The new Test should pass both before and after the change to the code.
Roger
On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
On 2/7/19 6:33 PM, David Holmes wrote:
On 8/02/2019 11:59 am, Ivan Gerasimov wrote: > Hi David! > > > On 2/7/19 5:16 PM, David Holmes wrote: >> Hi Ivan, >> >> On 8/02/2019 11:02 am, Ivan Gerasimov wrote: >>> Hello! >>> >>> The specification states: >>> """ >>> If the length of the specified CharSequence is less than or >>> equal to zero, then an empty buffer of capacity 16 is returned. >>> """ >>> >>> However, the current implementation throws either >>> NegativeArraySizeException or IndexOutOfBounds instead (the >>> actual exception type depends on the value of reported >>> negative length). >> >> How can you have a negative length CharSequence ?? >> > A custom CharSequence returning negative length() can be created. > Not sure how useful/popular this may be, though.
Seems pretty meaningless so just treating that as an error seems fine. Though somewhat debatable whether you need to add an appropriate @throws.
Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence.
(To be precise, there is one candidate, but I'll file a separate bug to fix it.)
> That's why I propose just removing the mentioning negative > length, and not changing the behavior. > > The proposed code change is to only unify the behavior for any > negative value of length.
Ok.
>> If its an empty CharSequence then it should return the empty >> buffer as indicated. >> > Empty CharSequence is processed correctly already.
Okay so by removing this part:
- * <p> - * If the length of the specified {@code CharSequence} is - * less than or equal to zero, then an empty buffer of capacity - * {@code 16} is returned.
you're relying on the main specification to implicitly handle the empty case.
Yes.
With kind regards, Ivan
David -----
> With kind regards, > Ivan > >> Cheers, >> David >> >>> It is proposed to do two things: >>> 1) remove the problematic sentence from the javadoc (CSR is >>> filed). >>> 2) unify the exception type thrown, if the argument reports >>> negative length. >>> NegativeArraySizeException will be consistent with, for >>> example, StringBuffer(negativeCapacity). >>> >>> Would you please help review the fix? >>> >>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 >>> WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ >>> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >>> >> >
On 2/12/19 7:33 AM, Roger Riggs wrote:
Hi Ivan,
The change to consistently throw NegativeSizeArrayException applies to StringBuilder also. Please mention that with the solution.
Right, thanks. I updated CSR accordingly. With kind regards, Ivan
Thanks, Roger
On 02/11/2019 06:12 PM, Ivan Gerasimov wrote:
Hi Roger!
On 2/11/19 7:30 AM, Roger Riggs wrote:
Hi Ivan,
I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior.
I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated.
Yes, this makes sense. I've updated CSR accordingly, and added a comment to the bug to make the intention of the fix clear.
Could you please review the updated CSR?
With kind regards, Ivan
Thanks, Roger
On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
Hi Roger!
That's because two tiny changes are combined in the patch:
1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior.
2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length in [-16, -1].
I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together.
With kind regards,
Ivan
On 2/8/19 1:49 PM, Roger Riggs wrote:
Hi Ivan,
In the direction of not changing the behavior; the webrev does change the behavior.
In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException.
AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents.
The new Test should pass both before and after the change to the code.
Roger
On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
On 2/7/19 6:33 PM, David Holmes wrote: > On 8/02/2019 11:59 am, Ivan Gerasimov wrote: >> Hi David! >> >> >> On 2/7/19 5:16 PM, David Holmes wrote: >>> Hi Ivan, >>> >>> On 8/02/2019 11:02 am, Ivan Gerasimov wrote: >>>> Hello! >>>> >>>> The specification states: >>>> """ >>>> If the length of the specified CharSequence is less than or >>>> equal to zero, then an empty buffer of capacity 16 is returned. >>>> """ >>>> >>>> However, the current implementation throws either >>>> NegativeArraySizeException or IndexOutOfBounds instead (the >>>> actual exception type depends on the value of reported >>>> negative length). >>> >>> How can you have a negative length CharSequence ?? >>> >> A custom CharSequence returning negative length() can be created. >> Not sure how useful/popular this may be, though. > > Seems pretty meaningless so just treating that as an error seems > fine. Though somewhat debatable whether you need to add an > appropriate @throws. > Right. There were two reasons not to add @throws here: - by removing the problematic paragraph we just make the javadoc almost the same as for StringBuilder(CharSequence), - we don't seem to have any other places (at least I couldn't find any) specifying exceptions due to negatively sized CharSequence.
(To be precise, there is one candidate, but I'll file a separate bug to fix it.)
>> That's why I propose just removing the mentioning negative >> length, and not changing the behavior. >> >> The proposed code change is to only unify the behavior for any >> negative value of length. > > Ok. > >>> If its an empty CharSequence then it should return the empty >>> buffer as indicated. >>> >> Empty CharSequence is processed correctly already. > > Okay so by removing this part: > > - * <p> > - * If the length of the specified {@code CharSequence} is > - * less than or equal to zero, then an empty buffer of > capacity > - * {@code 16} is returned. > > you're relying on the main specification to implicitly handle > the empty case. > Yes.
With kind regards, Ivan
> David > ----- > >> With kind regards, >> Ivan >> >>> Cheers, >>> David >>> >>>> It is proposed to do two things: >>>> 1) remove the problematic sentence from the javadoc (CSR is >>>> filed). >>>> 2) unify the exception type thrown, if the argument reports >>>> negative length. >>>> NegativeArraySizeException will be consistent with, for >>>> example, StringBuffer(negativeCapacity). >>>> >>>> Would you please help review the fix? >>>> >>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 >>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ >>>> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >>>> >>> >> >
-- With kind regards, Ivan Gerasimov
Hi Ivan, Sorry, one more. The compatibility risk description should be specific about the behavior that is changing. Something like: Since JDK5, the exception thrown for negative lengths have been unspecified and inconsistent; with this change, NegativeArraySizeException will be thrown. Thanks, Roger p.s. A release note capturing the essence of the change is probably needed also. Add label release-note=yes to the issue and create a subtask. On 02/12/2019 11:39 AM, Ivan Gerasimov wrote:
On 2/12/19 7:33 AM, Roger Riggs wrote:
Hi Ivan,
The change to consistently throw NegativeSizeArrayException applies to StringBuilder also. Please mention that with the solution.
Right, thanks. I updated CSR accordingly.
With kind regards, Ivan
Thanks, Roger
On 02/11/2019 06:12 PM, Ivan Gerasimov wrote:
Hi Roger!
On 2/11/19 7:30 AM, Roger Riggs wrote:
Hi Ivan,
I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior.
I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated.
Yes, this makes sense. I've updated CSR accordingly, and added a comment to the bug to make the intention of the fix clear.
Could you please review the updated CSR?
With kind regards, Ivan
Thanks, Roger
On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
Hi Roger!
That's because two tiny changes are combined in the patch:
1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior.
2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length in [-16, -1].
I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together.
With kind regards,
Ivan
On 2/8/19 1:49 PM, Roger Riggs wrote:
Hi Ivan,
In the direction of not changing the behavior; the webrev does change the behavior.
In the case of CharSeqence with length -1..-16, the new behavior throws NegativeArrayIndexException instead of java.lang.IndexOutOfBoundsException.
AbstractStringBuilder:101-102 throw an exception for length < 0. However, the current behavior is to create a StringBuffer with length + 16 and then append the contents. For -1..-16, it will succeed in creating a StringBuffer but fail with IndexOutOfBoundsException during the append of the contents.
The new Test should pass both before and after the change to the code.
Roger
On 02/07/2019 10:19 PM, Ivan Gerasimov wrote: > > > On 2/7/19 6:33 PM, David Holmes wrote: >> On 8/02/2019 11:59 am, Ivan Gerasimov wrote: >>> Hi David! >>> >>> >>> On 2/7/19 5:16 PM, David Holmes wrote: >>>> Hi Ivan, >>>> >>>> On 8/02/2019 11:02 am, Ivan Gerasimov wrote: >>>>> Hello! >>>>> >>>>> The specification states: >>>>> """ >>>>> If the length of the specified CharSequence is less than or >>>>> equal to zero, then an empty buffer of capacity 16 is returned. >>>>> """ >>>>> >>>>> However, the current implementation throws either >>>>> NegativeArraySizeException or IndexOutOfBounds instead (the >>>>> actual exception type depends on the value of reported >>>>> negative length). >>>> >>>> How can you have a negative length CharSequence ?? >>>> >>> A custom CharSequence returning negative length() can be created. >>> Not sure how useful/popular this may be, though. >> >> Seems pretty meaningless so just treating that as an error >> seems fine. Though somewhat debatable whether you need to add >> an appropriate @throws. >> > Right. There were two reasons not to add @throws here: > - by removing the problematic paragraph we just make the javadoc > almost the same as for StringBuilder(CharSequence), > - we don't seem to have any other places (at least I couldn't > find any) specifying exceptions due to negatively sized > CharSequence. > > (To be precise, there is one candidate, but I'll file a separate > bug to fix it.) > >>> That's why I propose just removing the mentioning negative >>> length, and not changing the behavior. >>> >>> The proposed code change is to only unify the behavior for any >>> negative value of length. >> >> Ok. >> >>>> If its an empty CharSequence then it should return the empty >>>> buffer as indicated. >>>> >>> Empty CharSequence is processed correctly already. >> >> Okay so by removing this part: >> >> - * <p> >> - * If the length of the specified {@code CharSequence} is >> - * less than or equal to zero, then an empty buffer of >> capacity >> - * {@code 16} is returned. >> >> you're relying on the main specification to implicitly handle >> the empty case. >> > Yes. > > With kind regards, > Ivan > > >> David >> ----- >> >>> With kind regards, >>> Ivan >>> >>>> Cheers, >>>> David >>>> >>>>> It is proposed to do two things: >>>>> 1) remove the problematic sentence from the javadoc (CSR is >>>>> filed). >>>>> 2) unify the exception type thrown, if the argument reports >>>>> negative length. >>>>> NegativeArraySizeException will be consistent with, for >>>>> example, StringBuffer(negativeCapacity). >>>>> >>>>> Would you please help review the fix? >>>>> >>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 >>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ >>>>> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >>>>> >>>> >>> >> >
Uh, lots of paperwork, comparing to the actual change in the source code :-) Anyways, I updated the CSR as you suggested, and created a release note subtask: https://bugs.openjdk.java.net/browse/JDK-8218884 Could you please review it? With kind regards, Ivan On 2/12/19 8:51 AM, Roger Riggs wrote:
Hi Ivan,
Sorry, one more. The compatibility risk description should be specific about the behavior that is changing. Something like:
Since JDK5, the exception thrown for negative lengths have been unspecified and inconsistent; with this change, NegativeArraySizeException will be thrown.
Thanks, Roger
p.s. A release note capturing the essence of the change is probably needed also. Add label release-note=yes to the issue and create a subtask.
On 02/12/2019 11:39 AM, Ivan Gerasimov wrote:
On 2/12/19 7:33 AM, Roger Riggs wrote:
Hi Ivan,
The change to consistently throw NegativeSizeArrayException applies to StringBuilder also. Please mention that with the solution.
Right, thanks. I updated CSR accordingly.
With kind regards, Ivan
Thanks, Roger
On 02/11/2019 06:12 PM, Ivan Gerasimov wrote:
Hi Roger!
On 2/11/19 7:30 AM, Roger Riggs wrote:
Hi Ivan,
I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior.
I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated.
Yes, this makes sense. I've updated CSR accordingly, and added a comment to the bug to make the intention of the fix clear.
Could you please review the updated CSR?
With kind regards, Ivan
Thanks, Roger
On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
Hi Roger!
That's because two tiny changes are combined in the patch:
1) remove a problematic statement from the javadoc, as it doesn't hold anyway. This part does not change the behavior.
2) unify type of the exception thrown for all negative values of CharSequence.length(). The regression test is to verify this, second change, so it fails prior the fix for length in [-16, -1].
I can separate them into different bugs, if you think it will make it clearer. Though I thought they can be fixed together.
With kind regards,
Ivan
On 2/8/19 1:49 PM, Roger Riggs wrote: > Hi Ivan, > > In the direction of not changing the behavior; the webrev does > change the behavior. > > In the case of CharSeqence with length -1..-16, the new behavior > throws NegativeArrayIndexException > instead of java.lang.IndexOutOfBoundsException. > > > AbstractStringBuilder:101-102 throw an exception for length < 0. > However, the current behavior is to create a StringBuffer with > length + 16 and > then append the contents. For -1..-16, it will succeed in > creating a StringBuffer > but fail with IndexOutOfBoundsException during the append of the > contents. > > The new Test should pass both before and after the change to the > code. > > Roger > > > On 02/07/2019 10:19 PM, Ivan Gerasimov wrote: >> >> >> On 2/7/19 6:33 PM, David Holmes wrote: >>> On 8/02/2019 11:59 am, Ivan Gerasimov wrote: >>>> Hi David! >>>> >>>> >>>> On 2/7/19 5:16 PM, David Holmes wrote: >>>>> Hi Ivan, >>>>> >>>>> On 8/02/2019 11:02 am, Ivan Gerasimov wrote: >>>>>> Hello! >>>>>> >>>>>> The specification states: >>>>>> """ >>>>>> If the length of the specified CharSequence is less than or >>>>>> equal to zero, then an empty buffer of capacity 16 is >>>>>> returned. >>>>>> """ >>>>>> >>>>>> However, the current implementation throws either >>>>>> NegativeArraySizeException or IndexOutOfBounds instead (the >>>>>> actual exception type depends on the value of reported >>>>>> negative length). >>>>> >>>>> How can you have a negative length CharSequence ?? >>>>> >>>> A custom CharSequence returning negative length() can be >>>> created. >>>> Not sure how useful/popular this may be, though. >>> >>> Seems pretty meaningless so just treating that as an error >>> seems fine. Though somewhat debatable whether you need to add >>> an appropriate @throws. >>> >> Right. There were two reasons not to add @throws here: >> - by removing the problematic paragraph we just make the >> javadoc almost the same as for StringBuilder(CharSequence), >> - we don't seem to have any other places (at least I couldn't >> find any) specifying exceptions due to negatively sized >> CharSequence. >> >> (To be precise, there is one candidate, but I'll file a >> separate bug to fix it.) >> >>>> That's why I propose just removing the mentioning negative >>>> length, and not changing the behavior. >>>> >>>> The proposed code change is to only unify the behavior for >>>> any negative value of length. >>> >>> Ok. >>> >>>>> If its an empty CharSequence then it should return the empty >>>>> buffer as indicated. >>>>> >>>> Empty CharSequence is processed correctly already. >>> >>> Okay so by removing this part: >>> >>> - * <p> >>> - * If the length of the specified {@code CharSequence} is >>> - * less than or equal to zero, then an empty buffer of >>> capacity >>> - * {@code 16} is returned. >>> >>> you're relying on the main specification to implicitly handle >>> the empty case. >>> >> Yes. >> >> With kind regards, >> Ivan >> >> >>> David >>> ----- >>> >>>> With kind regards, >>>> Ivan >>>> >>>>> Cheers, >>>>> David >>>>> >>>>>> It is proposed to do two things: >>>>>> 1) remove the problematic sentence from the javadoc (CSR is >>>>>> filed). >>>>>> 2) unify the exception type thrown, if the argument reports >>>>>> negative length. >>>>>> NegativeArraySizeException will be consistent with, for >>>>>> example, StringBuffer(negativeCapacity). >>>>>> >>>>>> Would you please help review the fix? >>>>>> >>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 >>>>>> WEBREV: >>>>>> http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ >>>>>> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >>>>>> >>>>> >>>> >>> >> > >
-- With kind regards, Ivan Gerasimov
Hi Ivan, The release note looks fine, Resolve -> FixDelivered to pass it on to the tech writer for review. Yes, communication is sometimes the hard part. Thanks, Roger On 02/12/2019 08:21 PM, Ivan Gerasimov wrote:
Uh, lots of paperwork, comparing to the actual change in the source code :-)
Anyways, I updated the CSR as you suggested, and created a release note subtask: https://bugs.openjdk.java.net/browse/JDK-8218884
Could you please review it?
With kind regards, Ivan
On 2/12/19 8:51 AM, Roger Riggs wrote:
Hi Ivan,
Sorry, one more. The compatibility risk description should be specific about the behavior that is changing. Something like:
Since JDK5, the exception thrown for negative lengths have been unspecified and inconsistent; with this change, NegativeArraySizeException will be thrown.
Thanks, Roger
p.s. A release note capturing the essence of the change is probably needed also. Add label release-note=yes to the issue and create a subtask.
On 02/12/2019 11:39 AM, Ivan Gerasimov wrote:
On 2/12/19 7:33 AM, Roger Riggs wrote:
Hi Ivan,
The change to consistently throw NegativeSizeArrayException applies to StringBuilder also. Please mention that with the solution.
Right, thanks. I updated CSR accordingly.
With kind regards, Ivan
Thanks, Roger
On 02/11/2019 06:12 PM, Ivan Gerasimov wrote:
Hi Roger!
On 2/11/19 7:30 AM, Roger Riggs wrote:
Hi Ivan,
I called it out because the CSR does not mention that the behavior of some of the cases (-1..-16) is changing and some of the emails asserted there was no change in behavior.
I'm fine with one changeset as long as both changes are explicit. The bug and the CSR should both be updated.
Yes, this makes sense. I've updated CSR accordingly, and added a comment to the bug to make the intention of the fix clear.
Could you please review the updated CSR?
With kind regards, Ivan
Thanks, Roger
On 02/08/2019 05:12 PM, Ivan Gerasimov wrote: > Hi Roger! > > That's because two tiny changes are combined in the patch: > > 1) remove a problematic statement from the javadoc, as it > doesn't hold anyway. This part does not change the behavior. > > 2) unify type of the exception thrown for all negative values of > CharSequence.length(). The regression test is to verify this, > second change, so it fails prior the fix for length in [-16, -1]. > > I can separate them into different bugs, if you think it will > make it clearer. Though I thought they can be fixed together. > > With kind regards, > > Ivan > > > On 2/8/19 1:49 PM, Roger Riggs wrote: >> Hi Ivan, >> >> In the direction of not changing the behavior; the webrev does >> change the behavior. >> >> In the case of CharSeqence with length -1..-16, the new >> behavior throws NegativeArrayIndexException >> instead of java.lang.IndexOutOfBoundsException. >> >> >> AbstractStringBuilder:101-102 throw an exception for length < 0. >> However, the current behavior is to create a StringBuffer with >> length + 16 and >> then append the contents. For -1..-16, it will succeed in >> creating a StringBuffer >> but fail with IndexOutOfBoundsException during the append of >> the contents. >> >> The new Test should pass both before and after the change to >> the code. >> >> Roger >> >> >> On 02/07/2019 10:19 PM, Ivan Gerasimov wrote: >>> >>> >>> On 2/7/19 6:33 PM, David Holmes wrote: >>>> On 8/02/2019 11:59 am, Ivan Gerasimov wrote: >>>>> Hi David! >>>>> >>>>> >>>>> On 2/7/19 5:16 PM, David Holmes wrote: >>>>>> Hi Ivan, >>>>>> >>>>>> On 8/02/2019 11:02 am, Ivan Gerasimov wrote: >>>>>>> Hello! >>>>>>> >>>>>>> The specification states: >>>>>>> """ >>>>>>> If the length of the specified CharSequence is less than >>>>>>> or equal to zero, then an empty buffer of capacity 16 is >>>>>>> returned. >>>>>>> """ >>>>>>> >>>>>>> However, the current implementation throws either >>>>>>> NegativeArraySizeException or IndexOutOfBounds instead >>>>>>> (the actual exception type depends on the value of >>>>>>> reported negative length). >>>>>> >>>>>> How can you have a negative length CharSequence ?? >>>>>> >>>>> A custom CharSequence returning negative length() can be >>>>> created. >>>>> Not sure how useful/popular this may be, though. >>>> >>>> Seems pretty meaningless so just treating that as an error >>>> seems fine. Though somewhat debatable whether you need to add >>>> an appropriate @throws. >>>> >>> Right. There were two reasons not to add @throws here: >>> - by removing the problematic paragraph we just make the >>> javadoc almost the same as for StringBuilder(CharSequence), >>> - we don't seem to have any other places (at least I couldn't >>> find any) specifying exceptions due to negatively sized >>> CharSequence. >>> >>> (To be precise, there is one candidate, but I'll file a >>> separate bug to fix it.) >>> >>>>> That's why I propose just removing the mentioning negative >>>>> length, and not changing the behavior. >>>>> >>>>> The proposed code change is to only unify the behavior for >>>>> any negative value of length. >>>> >>>> Ok. >>>> >>>>>> If its an empty CharSequence then it should return the >>>>>> empty buffer as indicated. >>>>>> >>>>> Empty CharSequence is processed correctly already. >>>> >>>> Okay so by removing this part: >>>> >>>> - * <p> >>>> - * If the length of the specified {@code CharSequence} is >>>> - * less than or equal to zero, then an empty buffer of >>>> capacity >>>> - * {@code 16} is returned. >>>> >>>> you're relying on the main specification to implicitly handle >>>> the empty case. >>>> >>> Yes. >>> >>> With kind regards, >>> Ivan >>> >>> >>>> David >>>> ----- >>>> >>>>> With kind regards, >>>>> Ivan >>>>> >>>>>> Cheers, >>>>>> David >>>>>> >>>>>>> It is proposed to do two things: >>>>>>> 1) remove the problematic sentence from the javadoc (CSR >>>>>>> is filed). >>>>>>> 2) unify the exception type thrown, if the argument >>>>>>> reports negative length. >>>>>>> NegativeArraySizeException will be consistent with, for >>>>>>> example, StringBuffer(negativeCapacity). >>>>>>> >>>>>>> Would you please help review the fix? >>>>>>> >>>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228 >>>>>>> WEBREV: >>>>>>> http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/ >>>>>>> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649 >>>>>>> >>>>>> >>>>> >>>> >>> >> >> >
participants (3)
-
David Holmes
-
Ivan Gerasimov
-
Roger Riggs