Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v4]
> The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Fixed the build of the native c++ test NullCallerTest to specify the c++ std library as part of the build. Changed the test to use iostream instead of printf. Enabled the test for Class::forName which is now located in test/jdk/jni/nullCaller (as part of the merge of JDK-8287171). - Merge branch 'master' into JDK-8281001 - make javadoc consistent with other caller sensitve methods - Added javadoc comment - Merge branch 'master' into JDK-8281001 - JDK-8281001 Examine the behavior of Class::forName if the caller is null - Changes: https://git.openjdk.java.net/jdk/pull/8711/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8711=03 Stats: 19 lines in 5 files changed: 10 ins; 3 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8711/head:pull/8711 PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v3]
> The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: make javadoc consistent with other caller sensitve methods - Changes: - all: https://git.openjdk.java.net/jdk/pull/8711/files - new: https://git.openjdk.java.net/jdk/pull/8711/files/9d054ea6..4e5f8c2b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8711=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8711=01-02 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8711/head:pull/8711 PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v2]
On Tue, 17 May 2022 20:16:48 GMT, Tim Prinzing wrote: >> The Class::forName behavior change to match JNI FindClass is a compatible >> change and seems pretty attractive as it would be expected that >> Class::forName would give the same behavior as FindClass which uses the >> system classloader. The test for 8281006 was enhanced to test for this >> change. Merged master to pick up fixes to unrelated test failures to reduce >> noise. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > Added javadoc comment src/java.base/share/classes/java/lang/Class.java line 361: > 359: * > 360: * > 361: * This method is caller sensitive. In cases where this method is > called You can drop "This method is caller sensitive." sentence for consistency with other caller-sensitive methods that do not state that explicitly. The javadoc already specifies that it uses the class loader of the current class. - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v2]
> The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Added javadoc comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/8711/files - new: https://git.openjdk.java.net/jdk/pull/8711/files/5b10f0d2..9d054ea6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8711=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8711=00-01 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8711/head:pull/8711 PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Tue, 17 May 2022 16:55:14 GMT, Tim Prinzing wrote: > I like the idea of moving all the null caller tests to a clearly named > directory to avoid duplication. +1. You can do this refactoring in a separate JBS issue and then update this PR when the tests are moved. - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Mon, 16 May 2022 18:55:42 GMT, Mandy Chung wrote: > `Class::forName(String)` javadoc should specify which class loader to use > when invoked with null caller similar to the other APIs you fixed for null > callers. > > I think this new test case does not belong to `NullCallerGetResource.java` > which is a test for `Module::getResource`. It's better to be placed under the > `test/jdk/java/lang/Class/forName` directory that makes it clear it's a test > for `Class::forName`. > > Alternatively, we could move all the tests for null caller under a new > clearly-named directory, maybe `test/jdk/jdk/nullCaller`. This may allow to > do some refactoring and remove the duplicated code in these test cases. What > do you think? I like the idea of moving all the null caller tests to a clearly named directory to avoid duplication. - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Sat, 14 May 2022 00:30:00 GMT, Tim Prinzing wrote: > The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. Please rename the title of the issue to reflect what is being proposed. - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Sat, 14 May 2022 00:30:00 GMT, Tim Prinzing wrote: > The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. `Class::forName(String)` javadoc should specify which class loader to use when invoked with null caller similar to the other APIs you fixed for null callers. I think this new test case does not belong to `NullCallerGetResource.java` which is a test for `Module::getResource`.It's better to be placed under the `test/jdk/java/lang/Class/forName` directory that makes it clear it's a test for `Class::forName`. Alternatively, we could move all the tests for null caller under a new clearly-named directory, maybe `test/jdk/jdk/nullCaller`.This may allow to do some refactoring and remove the duplicated code in these test cases. What do you think? src/java.base/share/classes/java/lang/Class.java line 385: > 383: ClassLoader loader = (caller == null) ? > 384: ClassLoader.getSystemClassLoader() : > 385: ClassLoader.getClassLoader(caller); Formatting nit: ClassLoader loader = (caller == null) ? ClassLoader.getSystemClassLoader() : ClassLoader.getClassLoader(caller); test/jdk/java/lang/module/exeNullCallerGetResource/NullCallerGetResource.java line 28: > 26: * @test > 27: * @bug 8281006 > 28: * @bug 8281001 Suggestion: * @bug 8281006 8281001 `@bug` can have one or more bug IDs - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Sat, 14 May 2022 00:30:00 GMT, Tim Prinzing wrote: > The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. @tprinzing , please file a CSR for this discuss so the behavioral changes can be reviewed. - PR: https://git.openjdk.java.net/jdk/pull/8711
