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

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

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