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&pr=8711&range=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&pr=8711&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8711&range=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&pr=8711&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8711&range=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
RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
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. - Commit messages: - 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&pr=8711&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281001 Stats: 19 lines in 3 files changed: 18 ins; 0 del; 1 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
