On 2015-09-14 09:24, Christian Thalinger wrote:
The JEP itself can be found here:
https://bugs.openjdk.java.net/browse/JDK-8062493
<https://bugs.openjdk.java.net/browse/JDK-8062493>
Here are the webrevs:
http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/
<http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/>
http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/
<http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/>
The change has already undergone a few iterations of internal review by
different people of different teams. Most comments and suggestions were
handled accordingly. Although there is one open item we agreed we will address
after the integration of JEP 243 and that work is captured in RFE:
https://bugs.openjdk.java.net/browse/JDK-8134994
<https://bugs.openjdk.java.net/browse/JDK-8134994>
SQE is still working on the tests and all test tasks can be seen as sub-tasks
of the JEP.
The integration will happen under the bug number:
https://bugs.openjdk.java.net/browse/JDK-8136421
<https://bugs.openjdk.java.net/browse/JDK-8136421>
Hi Christian,
(Adding build-dev since this review includes some major build changes.)
The makefile changes looks good in general to me. I have not reviewed
the source code changes.
Some comments:
* modules.xml:
Make sure you get sign-off from a Jigsaw developer for modifying this file.
* hotspot/make/linux/makefiles/gcc.make:
Seems unfortunate to have to disable a new warning
(undefined-bool-conversion) for newly incorporated code. Is it not
possible to fix the code emitting this warning instead?
* make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
I see a coming merge conflict. In jdk9/dev, there is now a new function
that performs the same function as CreatePath, but it's named PathList.
(It's a bit unfortunate that this code snippet has bounced around as
patches without a definite name.) I assume you are going to push this
through a hotspot forest. If the PathList patch reaches the hotspot repo
before this, please remove the CreatePath from MakeBase, and change the
calls to CreatePath to PathList instead. (I could only find such calls
in hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in
before that, we'll need to give a heads-up to the integrator about this
conflict.
Another potential coming merge conflict relates to ListPathsSafely in
Gensrc-java.base.gmk. There is currenlty a review out from Erik which
modifies the API for ListPathsSafely. If/when it goes in, the call to
ListPathsSafely in Gensrc-java.base.gmk will need to be modified (Erik
can give advice on how). Depending on timing, this too might hit the
integrator rather than your push.
/Magnus
