Hi Mandy,
On 01/11/16 23:42, Mandy Chung wrote:
Hi Daniel,
Here is the updated webrev incorporating your feedback:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.03
ClassLoader.java:
345 if (name != null && name.isEmpty()) {
346 throw new IllegalArgumentException("name must be
non-empty or null");
347 }
I'd suggest passing 'name' to 'checkCreateClassLoader()' and do
this check in checkCreateClassLoader instead - in order to do
the checks before 'this' is created.
[...]
Also should there be some asserts somewhere verifying that moduleVersion
is null or empty when moduleName is null or empty? At the moment the
constructor will blindly accept a version for an unnamed module,
and I am assuming this is wrong - am I right?
Good point. The constructor should throw IAE if module name/version and class
loader name is an empty string.
That's not exactly what I had in mind. I don't have
any particular feeling for whether "" should be allowed
or not. Maybe it would be simpler to just allow "" to mean the
same thing as 'null'. What I meant was:
Can moduleVersion be non null (and non empty) if moduleName is null
(or empty)? In other words can an unnamed module have a version?
Also if you add some validation to the constructor then
you will need to add a readObject to duplicate the validation
checks. And if you disallow "" then checking for isEmpty() in
toString() is not needed. But I have the feeling that simply
allowing "" and null to mean the same thing would be more
robust. It's just the question of having a moduleVersion
for an unnamed module that bothers me.
toLoaderModuleClassName will call ClassLoader.getName().
If a class loader overrides getName() then that might be a different
name than what StackTraceElement.getClassLoaderName() returns.
Is that an issue?
I added a test to verify StackTraceElement that uses the ClassLoader's name
field. I also added @apiNote in ClassLoader::getName.
Ok - the @apiNote should cover that.
Also I wonder whether you should be considering including a fix
for https://bugs.openjdk.java.net/browse/JDK-8167099 as part
of this change (though arguably the bug has been there since
new fields were added to StackTraceElement in 9).
It is related but it’s better to separate it from this issue. Do you have
cycle to help write a test case? I can help fix JDK-8167099 later.
Right - no problem with that.
I haven't given any thought to a test case for JDK-8167099.
I believe it will prevent to connect a JDK 9 jvisualvm to a
JDK 8 process if not fixed though. Maybe serializing a ThreadInfo
composite data containing some MonitorInfo in JDK 8, and
then deserializing that in JDK 9 and trying to convert it
back to ThreadInfo in 9 would show the issue.
best regards,
-- daniel
Mandy
