I have posted an updated webrev http://cr.openjdk.java.net/~bpb/7179567/webrev.3/ which I hope addresses all the concerns expressed below.
Please see also my comments in line. Once this looks acceptable I can do the CCC request. Thanks, Brian On Oct 7, 2013, at 9:28 PM, David Holmes wrote: > Aside: I'm confused about the relationship of this bug and JDK-6445180 - are > they not duplicates? Seems to me this one should have been closed as a dup > when it was reported. I think you are correct. I can adjust this once the webrev is approved. > On 5/10/2013 6:58 AM, Brian Burkhalter wrote: >> On Oct 3, 2013, at 5:38 PM, Brian Burkhalter wrote: >> >>> On Oct 3, 2013, at 5:35 PM, Alan Bateman wrote: >>> >>>> On 03/10/2013 16:10, Brian Burkhalter wrote: >>>>> Please review and comment at your convenience. >>>>> >>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-7179567 >>>>> Webrev: http://cr.openjdk.java.net/~bpb/7179567/ >> >> An updated webrev which I hope adequately addresses the expressed concerns >> may be found at: >> >> http://cr.openjdk.java.net/~bpb/7179567.2/ > > URLClassLoader.java: please delete the commented out line: > > + //Objects.requireNonNull(urls); I already fixed that but had not updated the webrev. > SecureClassLoader.java: > > 186 * @throws SecurityException if the {@code ClassLoader} instance is > not > 187 * initialized. > > should read "this classloader instance" as it refers to the current instance. > Also this may be bringing the spec into line with the implementation but > "initialization" here is purely an implementation detail not part of the > specification for this class so it should not be mentioned explicitly - and > this change still needs a CCC (which might help determine exactly what should > be said here - I'm unclear how you can try to call getPermissions if this is > uninitialized as it would need 'this' to escape from the constructor - which > perhaps it can do via a third-party security manager?). I've removed this @throws clause. > NoCallStackClassLoader.java: the comment is far too verbose, we don't write > such explanatory kinds of comments for this kind of thing (else the code > would be overrun with commentary!). Shortened. > Aside: if you are a JDK Author you don't need a Contributed-by line in the > commit comments. Removed. On Oct 8, 2013, at 4:08 AM, Alan Bateman wrote: > This looks much better. > > 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. > > 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). > > 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). Updated. On Oct 8, 2013, at 8:53 AM, Chris Hegarty wrote: > 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
