RFR 8218228 : The constructor StringBuffer(CharSequence) violates spec for negatively sized argument

[Roger Riggs]

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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>