RFR: 4247235 (spec str) StringBuffer.insert(int, char[]) specification is inconsistent (original) (raw)

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: >>>>>>>>>>>> *

Unless otherwise noted, passing a null 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