> 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) > > * 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
