On 10/08/2013 12:08 PM, Alan Bateman wrote:
On 04/10/2013 21:58, Brian Burkhalter wrote:
:
An updated webrev which I hope adequately addresses the expressed
concerns may be found at:
http://cr.openjdk.java.net/~bpb/7179567.2/
This looks much better.
Yes, this is much nicer.
I'd be tempted to write another private constructor, taking all args,
and checking params with checkForNullURLs before invoking, similar to
ThreadGroup [1] L117, for example. But that is just clean up to remove
some duplication. No need to do this.
If I read the code correctly then the long standing behavior of
URLClassLoader.getPermissions(null) was to throw a NPE, now it is
changed to return the empty collection in order to match the super type
(SecureClassLoader). I can't think of anything that would break so this
is probably okay, just maybe a bit surprising.
Agreed.
My previous comment on @throws vs. @exception was just to keep things
consistent as this is old code and would look odd for a method to use
both. Our preference is to switch to @throws whenever the opportunity
arises. The comment was also on the alignment/style. Take
URLCLassLoader(URL[], ClassLoader) for example, the existing
SecurityException has its description aligned under SecurityException
whereas the new NullPointerException wraps around. So my overall comment
here was to avoid mixing styles (it doesn't matter too much which style
is used, just keep it consistent for future maintainers).
Right, these classes are prime for stylistic clean up early in 9. For
now, keep things locally consistent ( even if that is old style ).
-Chris.
[1]
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/9f57d2774603/src/share/classes/java/lang/ThreadGroup.java
In the test then it might be better to change the @summary line to only
include the second paragraph (the bug summary is not interesting,
especially when it references a test that is not in OpenJDK).
-Alan.
