Hi Lois,
to make sure noone misses it, here's the new webrev:
http://cr.openjdk.java.net/~redestad/8232613/open.03
Comments inline..
On 2019-10-22 14:33, Lois Foltan wrote:
On 10/22/2019 7:13 AM, Claes Redestad wrote:
On 2019-10-22 13:05, Andrew Dinn wrote:
On 22/10/2019 12:05, Claes Redestad wrote:
On 2019-10-22 12:44, Andrew Dinn wrote:
Why not leave it void?
I guess:
http://cr.openjdk.java.net/~redestad/8232613/open.02/
Ship it ;-)
No wait, I missed the use in jni.cpp where the bool return is actually
used. I ignored it in systemDict since when I got an exception there
(by misspelling a name or using the wrong signature) the exception
bubbles up and crashes the VM.
Perhaps this could be reworked to remove the bool cleanly, but let's
go back to open.01 and move that cleanup to a follow-up.
/Claes
Hi Claes,
I do have a concern that this will be a behavioral change for method
resolution if Object::registerNatives is removed. For example,
public class RegisterNatives {
interface I { default public void registerNatives() {
System.out.println("I"); } }
static class A implements I { }
public static void main(String... args) {
A varA = new A();
varA.registerNatives();
}
Currently class A finds registerNatives in its superclass Object and the
test receives the following:
Exception in thread "main" java.lang.IllegalAccessError: class
RegisterNatives tried to access private method 'void
java.lang.Object.registerNatives()' (RegisterNatives is in unnamed
module of loader 'app'; java.lang.Object is in module java.base of
loader 'bootstrap')
at RegisterNatives.main(RegisterNatives.java:6)
With your change interface I's registerNatives default method is invoked
successfully. I don't think this is a major backward compatibilty issue
but we should have someone from core-libs okay the removal of the method
from Object before committing. In addition, can you add this test as
part of your change? I think it would be okay to put it in
open/test/hotspot/jtreg/runtime/8024804 which contains an existing
registerNatives test.
Yeah, *not* throwing an IAE on this feels like an unintentional bug fix.
:-)
We're relaxing a very subtle interaction, so I think compatibility
issues with existing code is non-existing. What could be an issue is
that someone might implement this pattern and then see it no longer
working on older versions of the OpenJDK.
However, that's already a possible scenario since the sample already
runs fine on some other non-OpenJDK JCK-compliant java implementations.
Thus I think this compatibility observation just adds an argument _in
favor_ of this RFE.
I've updated the test you pointed me to to include your test scenario.
Thanks!
/Claes
Otherwise I think .01 from the Hotspot side looks good. Only one minor
comment:
method.cpp
- line #426 and 427: Since you are in this code can your change to
resourceMark(THREAD)?
Thanks,
Lois
