On 22/10/2019 11:41 pm, Claes Redestad wrote:
Hi Lois,
to make sure noone misses it, here's the new webrev:
http://cr.openjdk.java.net/~redestad/8232613/open.03
I like the code placement in this version - Coleen's suggestion was
better than mine :)
Comments inline..
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,
Good catch Lois!
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.
:-)
Yes it is a somewhat surprising aspect of default method resolution, but
methods in the class hierarchy must be considered ahead of any default
method - even inaccessible ones.
We're relaxing a very subtle interaction, so I think compatibility
issues with existing code is non-existing.
I don't think there is a compatibility issue here because we don't in
general have to maintain error compatibility. However as it is a change
in behaviour it does warrant a CSR request just so the compatibility
argument is captured/recorded.
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.
Unfortunate, but not a compatibility scenario we have to support.
Cheers,
David
-----
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
