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 <claes.redes...@oracle.com> 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 >>>
