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