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

[Ivan Gerasimov]

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