On 2015-09-16 22:25, Christian Thalinger wrote:
On Sep 16, 2015, at 10:11 AM, Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com
<mailto: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
<mailto: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/%7Ekvn/JVMCI/webrev.top/>
<http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/
<http://cr.openjdk.java.net/%7Ekvn/JVMCI/webrev.top/>>
http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/
<http://cr.openjdk.java.net/%7Ekvn/JVMCI/webrev.hotspot/>
<http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/
<http://cr.openjdk.java.net/%7Ekvn/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?
It's more a hotspot code quality issue, not a build system issue, so I
won't insist. I just wanted to point out that this change will start
hiding a new kind of warning for all files in hotspot. Unless there was
a compelling reason, I would personally rather see an effort to fix the
code in question. But if no-one from Hotspot agrees on this, I'll drop it.
/Magnus
