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.dui...@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.g...@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.g...@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.g...@oracle.com
