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
