> On Sep 16, 2015, at 6:52 AM, Christian Thalinger > <christian.thalin...@oracle.com> 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) > >> >> * 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.
Erik sent me a patch to avoid merge conflicts. I’ve integrated two changesets: http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab <http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 <http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9> > >> >> 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 >
