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

[Ivan Gerasimov]

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