On 2015-09-16 18:52, Christian Thalinger wrote:
On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com>
wrote:
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.
I’ve asked Alan to take a look.
* 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?
We had this question before. It’s not obvious because you can’t see it in the
regular diff views but this is under:
ifeq ($(USE_CLANG), true)
I'm not sure I understand why that's relevant..? Isn't it just as
important to try to submit warning-free code when compiling with clang
as with any other compiler? Or is clang just being anal about the code
in question?
/Magnus
* 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.
Thanks for the heads up.
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.
Ok, thanks.
/Magnus
