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.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
