Thanks Mandy, Claes,
This is for real now ;-)
Regards, Peter
On 01/05/2017 03:05 AM, Mandy Chung wrote:
I reviewed webrev.09to10. The NPE check is fine. I also ran the core tests
and no surprise found.
Mandy
On Jan 4, 2017, at 8:30 AM, Claes Redestad <[email protected]> wrote:
Hi,
no new failures - seems good to go unless someone objects to the
NPE behavior change.
Thanks!
/Claes
On 2017-01-03 18:15, Claes Redestad wrote:
Hi Peter,
letting you know I've submitted a build and test job for this to run
over night.
Thanks!
/Claes
On 01/03/2017 05:52 PM, Peter Levart wrote:
Hi,
Now that initialization cycle has been broken by Claes' fix for:
https://bugs.openjdk.java.net/browse/JDK-8172048
I prepared a revised fix for issues described in the following bugs:
https://bugs.openjdk.java.net/browse/JDK-8062389
https://bugs.openjdk.java.net/browse/JDK-8029459
https://bugs.openjdk.java.net/browse/JDK-8061950
...now tracked by bug:
https://bugs.openjdk.java.net/browse/JDK-8172190
The revised webrev is here:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.10/
The incremental change from webrev.09 is the following:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.09to10/
Besides fixing one failing test (StarInheritance) that now has to
account for new behavior of Class.getMethods(), some tests also failed
because of the changed behavior (not throwing NPE) when passing null
'name' parameter to Class.get[Declared][Method|Field] methods. This
was a straight bug introduced by me and caught by tests which I
haven't re-run after introducing it. I apologize for that and promise
to be more careful in the future. In original code the NPE was thrown
by private Class.searchFields and searchMethods in the part of code
where the 'name' parameter was interned. I replaced interning and
reference comparison with equality comparison, which is faster and
does not trash the String interning cache with names that are later
not found as members of classes, but I forgot to introduce explicit
nonNull check for the 'name' argument.
I now intentionally inserted the nonNull checks in front of the
security checks in public-facing methods although this introduces a
little behavioral change. I choose that because:
- The javadocs list NullPointerException before the SecurityException
(for example in getField):
* @throws NullPointerException if {@code name} is {@code null}
* @throws SecurityException
* If a security manager, <i>s</i>, is present and
* the caller's class loader is not the same as or an
* ancestor of the class loader for the current class and
* invocation of {@link SecurityManager#checkPackageAccess
* s.checkPackageAccess()} denies access to the package
* of this class.
- I think it is more correct for methods to 1st check parameters
before throwing any other state related exceptions.
But if you think this behavioral change is not justified, I can
reverse the order of checks and prepare new webrev.
I have successfully executed java/lang tests and all tier 1 tests that
failed when webrev.09 was pushed, mentioned in:
https://bugs.openjdk.java.net/browse/JDK-8171988
But don't take my word for it. Can someone please run webrev.10
through the tests on Oracle side before confirming the change?
Thanks,
Peter Levart
