Alan,
Thank you for the review!
Just wanted to notify you that I'm officially on vacation this week as
all US employees.
But I'm at home so that I can participate in the email discussions.
The next week I'm on vacation as well and will be out of town with
limited access to the internet.
Please, see my comments below.
On 12/28/15 08:22, Alan Bateman wrote:
On 23/12/2015 06:24, serguei.spit...@oracle.com wrote:
Please, review this initial fix for the Jigsaw Bill milestone task:
https://bugs.openjdk.java.net/browse/JDK-8049365
Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.1/
Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.1/
Summary:
This round includes most of the changes suggested or discussed by
Alan in the round #0:
- The version 1.9 is replaced version
- The Module interface is replaced with the ModuleReference
interface that extends the ObjectReference.
- The argument name "target" in the CanRead is replaced with
"source" to make it consistent with
the class java.lang.reflect.Module.
- The public field TRACE_MODULES has been removed.
- Some other small changes.
I hope, all comments are addressed. But, please, let me know
if anything is missed.
This round does not include the corresponding update of the SA-JDI.
I'm suggesting to separate and postpone this part for now to speed
up the main part.
Another reason for it is that the SA-JDI update depends on possible
VM changes.
This round adds new capability "can_get_modules_info" that is
appeared on all levels:
JVMTI, JDWP and JDI.
No problem updating SA-JDI at a later time, we just need to make sure
it builds without issues.
Good.
I'll open a task for that.
I looked through the updated webrevs and it looks quite good. A few
comments:
It's not clear to me that a new JVM TI capability is required. I agree
that JDWP and JDI need a canXXX command/method but if we rev JVM TI to
version 9 then the VM supports modules and so a capability shouldn't
be needed.
It would be nice to get rid of this JVM TI capability.
But my question is if new JVMTI functionality is mandatory for all VM's
out there.
The JDWP command is currently "canGetModulesInfo", it might be better
as "canGetModuleInfo". Ditto for the method on VirtualMachine.
Taken.
I was also thinking if this can be better..
The JDWP agent uses JNI to upcall to Module::getClassLoader and
Module::canRead, we might consider adding JNI or JVM TI functions in
the future and avoid this.
I was thinking about the same.
My preference would be to add new JVM TI functions.
VirtualMachine::allModules - there's a spurious <UL> in the javadoc.
Will check and clean it up.
ModuleReference javadoc currently contains ".. has a read edge with
the source ModuleReference". We'll need to do a bit of wordsmithing on
the javadoc but for this one then starting out with " .. if the module
reads the source module" would be clearer.
Taken.
Thank you for the correction.
InvalidModuleException - the copyright date says 1998 :-)
Strange.
I've already fixed that. :)
I see you have a basic test. The checkClassLoader method needs
comments as it's not clear what it is testing. We'll need to add
further tests to cover all the scenarios, the unnamed module case for
example.
Yes, of course.
I'll try to enhance the test case.
Thanks!
Serguei
-Alan
