JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE

[Chris Hegarty]

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