Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v4]

2022-06-02 Thread Tim Prinzing
> 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]

2022-05-26 Thread Tim Prinzing
> 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]

2022-05-25 Thread Mandy Chung
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]

2022-05-17 Thread Tim Prinzing
> 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

2022-05-17 Thread Mandy Chung
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

2022-05-17 Thread Tim Prinzing
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

2022-05-16 Thread Ioi Lam
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

2022-05-16 Thread Mandy Chung
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

2022-05-13 Thread Joe Darcy
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

2022-05-13 Thread Tim Prinzing
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=8711=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