RFR: 4247235 (spec str) StringBuffer.insert(int, char[]) specification is inconsistent
Jim Gish
jim.gish at oracle.com
Tue Jan 22 19:54:30 UTC 2013
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