Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/8e9e3542...0bd5b477 lgtm - Marked as reviewed by vromero (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715396892
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/baf8d59f...0bd5b477 Changes in the two source launcher files `Main.java` and `SourceLauncherTest.java` look good to me - less code is more! - PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715230089
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/baf8d59f...0bd5b477 Javac changes look good - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715227587
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/caa2870d...0bd5b477 javac changes look reasonable to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715097616
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 15:00:41 GMT, Jim Laskey wrote:
>> src/java.base/share/native/libjli/java.c line 559:
>>
>>> 557:
>>> 558: /* Build platform specific argument array */
>>> 559: if ((mainType & MAIN_WITHOUT_ARGS) == 0) {
>>
>> nice, thanks, a minor addition to the condition: `&& argc > 0`?
>
> Need an empty array when argc == 0.
oh I see
>> src/java.base/share/native/libjli/java.c line 561:
>>
>>> 559: if ((mainType & MAIN_WITHOUT_ARGS) == 0) {
>>> 560: mainArgs = CreateApplicationArgs(env, argv, argc);
>>> 561: CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
>>
>> side: this is just a comment not proposing any change, I know it is a good
>> practice to check for nulls here, but having a null here is really possible?
>> how can we have a null here? but I guess this also check for any other
>> exception right?
>
> Yes it's a combo thing. This was the code that was there BTW.
yes I know it is previous code, I was just wondering how we could have a null
exception here but it is true that other exceptions can occur
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1381846743
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1381848172
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 14:26:29 GMT, Vicente Romero wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'upstream/master' into 8315458
>> - Don't get args unless necessary
>> - Remove unnamed classes from examples.not-yet.txt
>> - Requested corrections
>> - Changes recommended by Jan
>> - Revised implicit class test
>> - Don't store main method info globally. Use addition calls to fetch info.
>> - Update JEP number in PreviewFeature
>> - Remove MANDATED flag from implicit classes
>> - Remove .orig files
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/64f1c416...0bd5b477
>
> src/java.base/share/native/libjli/java.c line 559:
>
>> 557:
>> 558: /* Build platform specific argument array */
>> 559: if ((mainType & MAIN_WITHOUT_ARGS) == 0) {
>
> nice, thanks, a minor addition to the condition: `&& argc > 0`?
Need an empty array when argc == 0.
> src/java.base/share/native/libjli/java.c line 561:
>
>> 559: if ((mainType & MAIN_WITHOUT_ARGS) == 0) {
>> 560: mainArgs = CreateApplicationArgs(env, argv, argc);
>> 561: CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
>
> side: this is just a comment not proposing any change, I know it is a good
> practice to check for nulls here, but having a null here is really possible?
> how can we have a null here? but I guess this also check for any other
> exception right?
Yes it's a combo thing. This was the code that was there BTW.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1381823409
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1381824589
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote:
>> Address changes from JEP 445 to JEP 463.
>>
>> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class.
>>
>> - Don't mark class on read.
>>
>> - Remove reflection and annotation processing related to unnamed classes.
>>
>> - Simplify main method search.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 12 additional commits since
> the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into 8315458
> - Don't get args unless necessary
> - Remove unnamed classes from examples.not-yet.txt
> - Requested corrections
> - Changes recommended by Jan
> - Revised implicit class test
> - Don't store main method info globally. Use addition calls to fetch info.
> - Update JEP number in PreviewFeature
> - Remove MANDATED flag from implicit classes
> - Remove .orig files
> - ... and 2 more: https://git.openjdk.org/jdk/compare/8f2f4091...0bd5b477
src/java.base/share/native/libjli/java.c line 559:
> 557:
> 558: /* Build platform specific argument array */
> 559: if ((mainType & MAIN_WITHOUT_ARGS) == 0) {
nice, thanks
src/java.base/share/native/libjli/java.c line 561:
> 559: if ((mainType & MAIN_WITHOUT_ARGS) == 0) {
> 560: mainArgs = CreateApplicationArgs(env, argv, argc);
> 561: CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
side: this is just a comment not proposing any change, I know it is a good
practice to check for nulls here, but having a null here is really possible?
how can we have a null here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1381763339
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1381775757
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
> Address changes from JEP 445 to JEP 463. > > - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. > > - Don't mark class on read. > > - Remove reflection and annotation processing related to unnamed classes. > > - Simplify main method search. Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8315458 - Don't get args unless necessary - Remove unnamed classes from examples.not-yet.txt - Requested corrections - Changes recommended by Jan - Revised implicit class test - Don't store main method info globally. Use addition calls to fetch info. - Update JEP number in PreviewFeature - Remove MANDATED flag from implicit classes - Remove .orig files - ... and 2 more: https://git.openjdk.org/jdk/compare/f04a1529...0bd5b477 - Changes: - all: https://git.openjdk.org/jdk/pull/16461/files - new: https://git.openjdk.org/jdk/pull/16461/files/4b3c03fe..0bd5b477 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16461=11 - incr: https://webrevs.openjdk.org/?repo=jdk=16461=10-11 Stats: 23833 lines in 485 files changed: 11893 ins; 4870 del; 7070 mod Patch: https://git.openjdk.org/jdk/pull/16461.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16461/head:pull/16461 PR: https://git.openjdk.org/jdk/pull/16461
