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

[Roger Riggs]

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