On 1/21/16 04:51, Alan Bateman wrote:
On 20/01/2016 22:35, serguei.spit...@oracle.com wrote:
:
Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8049365-Jigsaw-jdk.2/
Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8049365-Jigsaw-hs.2/
jvmti.xml - the module catagory has since="9" but the JVM TI spec
version rev's to 1.3.2. I started a thread a few weeks ago on whether
JNI and JVM TI should move to major version 9 and just track the Java
SE version going forward. I don't think we have closure on that
discussion yet but for now then I suggest we move to JVMTI_VERSION_9
on the assumption that this is the way forward.
Ok
I saw the discussion but have no strong opinion yet.
Changing to JVMTI_VERSION_9 will make it more consistent though.
The GetAllModules implementation is okay for now and I believe Lois
will replace JvmtiModuleClosure once your changes are in and she gets
time to replace it.
I hope so.
Now to the jdk repo ...
We seem to have an issue with the generated jvmti.h generating a GPL
copyright header. This means that copying jvmti.h into the jdk repo
will change it from GPL + "Classpath" exception to pure GPL. I'll
create a separate bug for this, this seems to be technical debt that
we have a copy of jvmti.h in the jdk repo but fixing that issue means
we need a jvmti.h with the right copyright because that is what goes
into the JDK include directory.
I could easily fix it now but it is Ok to file a separate bug.
jdi.ReferenceType - I didn't generate the javadoc with your patch but
it looks like the "Not all target ..." note will end up in the @return
tag and I assume should move up. There is also a stray after the
@throws.
Ok
jdi.VirtualMachine - this has the same issue with the "Not all target
..." note. In the canGetModuleInfo() method then I assume the @see
should be moved to the end of the class description.
Ok
Can you check allModules in VirtualMachineImpl.c as it doesn't look
quite right when GetAllModules fails, is there a return missing as
otherwise it will write the module count and attempt to deallocate NULL.
Will check.
@jdk.Exported has been removed in JDK 9 and that removal has got to
jake. I assume your forest is a bit behind.
Nice catch.
Will pull an update.
jdi.ModuleReference
- as this code is new then it would be good to use {@code ..} instead
of .
- I don't think name, classLoader or canRead can throw UOE so this
exception can be removed from the javadoc.
- the class description reads "A module that currently exists in the
target VM". Looking at the other JDI classes then it might be more
consistent to use "A module in the target VM".
Ok
tools.jdi.ModuleReferenceImpl
- if change the fields to volatile then we can avoid these methods
needing to be synchronized.
- a minor nit is that it would be good to avoid overly long lines as
it's a pain to have v. long lines when doing side-by-side diffs.
Ok
tools.jdi.VirtualMachineImpl
- can modulesByID be Map, I can't quite tell
why the value needs to be the impl type.
- It looks like vmMajorVersion will return 0 when the target VM is JDK
8 or older, is that right? I guess that is okay because we only care
about >= 9 but we will need to look at this closely.
Will check.
As per previous mails then I think we'll need additional tests in this
area. It would be good to create another issue in JIRA to track that work.
Agreed.
Will provide an update in the next review round.
Thanks,
Serguei
-Alan
