> On Sep 16, 2015, at 10:11 AM, Magnus Ihse Bursie > <magnus.ihse.bur...@oracle.com> wrote: > > 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?
I don’t have a Clang compiler at hand but Clang is anal about everything. Do you want that suppression to be removed? > > /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
