RFR: 4247235 (spec str) StringBuffer.insert(int, char[]) specification is inconsistent
Jim Gish
jim.gish at oracle.com
Wed Jan 23 17:08:58 UTC 2013
This time with the attachment...
Thanks,
Jim
On 01/22/2013 02:54 PM, Jim Gish wrote:
> change set/patch attached for pushing.
>
> Thanks,
> Jim
> On 01/22/2013 02:40 PM, Jim Gish wrote:
>> I've made the changes as suggested by Mike, and this has now been
>> approved by CCC. Could someone please give it one more look and push
>> it for me, please?
>>
>> Thanks,
>> Jim
>>
>> http://cr.openjdk.java.net/~jgish/Bug4247235-Add-Blanket-Null-Stmt/
>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-Add-Blanket-Null-Stmt/>
>>
>> On 01/10/2013 11:00 AM, Jim Gish wrote:
>>> Stephen,
>>> Currently here's (a sampling) of how the other methods work.
>>> Although most check indices first, not all do... (The original bug
>>> was about this very inconsistency, however one answer given to it
>>> was that in general we don't say anything about order of
>>> exceptions). However, given that most of the methods do index
>>> checking first, I think I agree with Mike on this one.
>>>
>>> Jim
>>>
>>> getChars(int srcBegin, int srcEnd, char[]dst, int dstBegin)
>>> - will check for srcBegin, srcEnd first and throw
>>> StringIndexOutOfBoundsException
>>> - then System.arraycopy checks for null and throws NPE
>>>
>>> replace(int start, int end, String str)
>>> - same as above (checking start and end first)
>>>
>>> insert(int index, char[] str, int offset, int len)
>>> - same as above (checking index and offset first)
>>>
>>> insert(int offset, char[] str)
>>> - same (checking offset first)
>>>
>>> String(char value[], int offset, int count)
>>> - same
>>> String(int[] codePoints, int offset, int count)
>>> - same
>>> String(byte ascii[], int hibyte, int offset, int count)
>>> - same
>>> String(byte bytes[], int offset, int length, String
>>> charsetName)
>>> - same
>>>
>>> * String(byte bytes[], int offset, int length, Charset charset)
>>> - checks charset == null first!
>>> - then checks offset and length
>>> - and then checks bytes==null
>>>
>>> String.getChars(char dst[], int dstBegin)
>>> - checks for dst==null first
>>> - then for bad and throw ArrayIndexOutOfBoundsException
>>>
>>>
>>> On 01/10/2013 06:47 AM, Stephen Colebourne wrote:
>>>> I would encourage null checking to be done first, rather than
>>>> sometimes getting StringIndexOutOfBoundsException for a null input.
>>>> Reasoning about the JDK methods is much easier that way around.
>>>>
>>>> Stephen
>>>>
>>>>
>>>> On 9 January 2013 23:09, Mike Duigou <mike.duigou at oracle.com> wrote:
>>>>> AbstractStringBuilder probably needs the "Unless otherwise noted,"
>>>>> blanket statement as well (Same as StringBuffer/StringBuilder)
>>>>>
>>>>> You might want to move the Objects.requireNonNull(dst); in
>>>>> String.getBytes() to after the existing checks so as not to
>>>>> unnecessarily change the exception thrown for bad inputs. An
>>>>> exception will still be thrown but some bad code may anticipate
>>>>> StringIndexOutOfBoundsException rather than NPE. This is a very
>>>>> minor matter though.
>>>>>
>>>>> Otherwise, it looks good.
>>>>>
>>>>> Mike
>>>>>
>>>>> On Jan 9 2013, at 14:46 , Jim Gish wrote:
>>>>>
>>>>>> Please review the following:
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~jgish/Bug4247235-Add-Blanket-Null-Stmt/
>>>>>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-Add-Blanket-Null-Stmt/>
>>>>>>
>>>>>> Here's a specdiff:
>>>>>> http://cr.openjdk.java.net/~jgish/Bug4247235-string-specdiff/
>>>>>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/>
>>>>>>
>>>>>>
>>>>>> Summary: This change replaces java/lang/*String*.java javadoc,
>>>>>> method-specific @throws NullPointerException clauses with
>>>>>> blanket null-handling statements like that currently in String.java
>>>>>>
>>>>>> That was motivated by a discussion awhile back, strongly favoring
>>>>>> a blanket statement over individual @throws clauses.
>>>>>>
>>>>>> For String, the following blanket statement is already in place:
>>>>>>
>>>>>> * <p> Unless otherwise noted, passing a <tt>null</tt> argument to
>>>>>> a constructor
>>>>>> * or method in this class will cause a {@link
>>>>>> NullPointerException} to be
>>>>>> * thrown.
>>>>>>
>>>>>>
>>>>>> However, despite the blanket statement, the following methods
>>>>>> also have @throws clauses:
>>>>>>
>>>>>> public boolean contains(java.lang.CharSequence s)
>>>>>>
>>>>>> Throws:
>>>>>> |java.lang.NullPointerException|- if|s|is|null|
>>>>>> ---------------------------------------------------------------
>>>>>>
>>>>>> public staticString
>>>>>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html>
>>>>>> format(String
>>>>>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html>
>>>>>> format,
>>>>>> java.lang.Object... args)
>>>>>>
>>>>>>
>>>>>> Throws:
>>>>>> |java.lang.NullPointerException|- If the|format|is|null
>>>>>> |-----------------------------------------------------------------------
>>>>>>
>>>>>> ||
>>>>>>
>>>>>> public staticString
>>>>>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html>
>>>>>> format(java.util.Locale l,
>>>>>> String
>>>>>> <http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html>
>>>>>> format,
>>>>>> java.lang.Object... args)
>>>>>>
>>>>>> Throws:
>>>>>> |java.lang.NullPointerException|- If the|format|is|null
>>>>>> |--------------------------------------------------------------------------
>>>>>>
>>>>>> All of the above are properly specified with the blanket
>>>>>> statement or other parts of their spec (such as format w.r.t.
>>>>>> null Locale) and the @throws can safely be removed.
>>>>>>
>>>>>> Because the blanket statement is already in effect for
>>>>>> String.java, I wrote tests for all methods/constructors to ensure
>>>>>> compliance. Running them revealed the following:
>>>>>>
>>>>>> public void getBytes(int srcBegin, int srcEnd, byte dst[], int
>>>>>> dstBegin)
>>>>>> - I would expect an NPE to be thrown if dst == null. However,
>>>>>> this isn't always the case. If dst isn't accessed because of the
>>>>>> values of the other parameters (as in
>>>>>> getBytes(1,2,(byte[]null,1), then no NPE is thrown.
>>>>>> - This brings up the question as to the correctness of the
>>>>>> blanket statement w.r.t. this method. I think this method (and
>>>>>> others like it) should use Objects.requireNonNull(dst). (The
>>>>>> correspoding method public void getChars(int srcBegin, int
>>>>>> srcEnd, char dst[], int dstBegin), does always throw NPE for
>>>>>> dst==null)
>>>>>>
>>>>>> All other methods/constructors in StringBuffer and StringBuilder
>>>>>> either correctly state null-handling behavior that differs from
>>>>>> the blanket statement or are correct w.r.t. the blanket statement.
>>>>>>
>>>>>> This has been reviewed by the JCK team. It will require CCC
>>>>>> approval (because of the new blanket statement, and the fact that
>>>>>> some of the existing methods were previously silent on null
>>>>>> handling, but all already conform to the blanket statement).
>>>>>>
>>>>>> Thanks,
>>>>>> Jim Gish
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
>>>>>> Oracle Java Platform Group | Core Libraries Team
>>>>>> 35 Network Drive
>>>>>> Burlington, MA 01803
>>>>>> jim.gish at oracle.com
>>>>>>
>>>>>> --
>>>>>> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
>>>>>> Oracle Java Platform Group | Core Libraries Team
>>>>>> 35 Network Drive
>>>>>> Burlington, MA 01803
>>>>>> jim.gish at oracle.com
>>>>>>
>>>
>>
>
--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.gish at oracle.com
More information about the core-libs-dev
mailing list