Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v8]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Anonymous main classes renamed to unnamed classes - Add test - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/a09a0a1b..ff7cd4c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=07 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=06-07 Stats: 314 lines in 14 files changed: 151 ins; 135 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 18:00:38 GMT, Jim Laskey wrote: >> src/java.base/share/native/libjli/java.c line 590: >> >>> 588: CHECK_EXCEPTION_NULL_LEAVE(mainID); >>> 589: (*env)->CallVoidMethod(env, mainObject, mainID); >>> 590: break; >> >> This calls into LauncherHelper to get the "main type", then calls the static >> or new/instance method. I'm wondering if you tried have a single entry point >> in LauncherHelper instead. I think the only downside might be that the >> trampoline would show up in stack traces but @Hidden could hide that. > > Good idea. If @hidden doesn't work then we can eat the trace entries. Many dependencies (ex. JDI) on main being the first frame. Will set up a separate RFE witch should include using the LauncherHelper in the source code launcher. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1181532559
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v7]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Move AnonymousMainClass to parser - Revert java launch - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/2c321834..a09a0a1b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=06 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=05-06 Stats: 231 lines in 4 files changed: 111 ins; 108 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Fri, 28 Apr 2023 17:51:58 GMT, Jan Lahoda wrote: >> I believe we were discussing this some time ago, and there were some >> problems. I don't recall the exact details, but I'll try to look into this >> later. > > I've sketched this: > https://github.com/lahodaj/jdk/commit/efe55f7d354ed7bbf91077d058823d682db501b9 > > I don't have too strong opinion here, there might be more cleanup possible > after a change like this, and might be a bit cleaner outside of the parser; > but forces the parser to work with files which is somewhat less clean. This is basically what I did originally until I decided that I needed a distinct class name. `log.currentSourceFile()` is the magic in this code. I'm willing to move this way if we all agree. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180715358
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v6]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Leave exception alone - Unused variables - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/e5ca303a..2c321834 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=05 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=04-05 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Fri, 28 Apr 2023 13:01:33 GMT, Jan Lahoda wrote: >> I see that logic in JavaCompiler - I wonder if that's just the way it is, or >> if there's a deeper reason as to why the sourcefile is set on the toplevel >> unit *after* parsing (I don't think I can see any, in which case that might >> be changed if that makes the rest of the code simpler). @lahodaj what do you >> think? > > I believe we were discussing this some time ago, and there were some > problems. I don't recall the exact details, but I'll try to look into this > later. I've sketched this: https://github.com/lahodaj/jdk/commit/efe55f7d354ed7bbf91077d058823d682db501b9 I don't have too strong opinion here, there might be more cleanup possible after a change like this, and might be a bit cleaner outside of the parser; but forces the parser to work with files which is somewhat less clean. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180673509
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v5]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Can't be invokeExact for instance main - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/f66f6e49..e5ca303a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v4]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Recommended changes #1 - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/53a5321d..f66f6e49 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=02-03 Stats: 222 lines in 11 files changed: 58 ins; 126 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Fri, 28 Apr 2023 14:26:10 GMT, Jan Lahoda wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 14 commits:
>>
>> - Merge branch 'master' into 8306112
>> - PreviewFeatures.isEnabled()
>> - Clean up isPreview
>> - Missing exception
>> - Corrections
>> - Update VM.java
>> - Clean up testing
>> - Update TestJavacTaskScanner.java
>> - Merge branch 'master' into 8306112
>> - Clean up
>> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java
> line 3933:
>
>> 3931: if
>> (Feature.ANONYMOUS_MAIN_CLASSES.allowedInSource(source) &&
>> 3932: !isDeclaration() &&
>> 3933: (token.kind == VOID || token.kind ==
>> IDENTIFIER)) {
>
> These checks (`token.kind == VOID || token.kind == IDENTIFIER`) for token
> kind will fail to parse fields of primitive types (or methods with primitive
> types as a return type)? Either the tests must include the primitive types,
> or (maybe) just this test can be avoided completely?
Things have moved on since. Changing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180512100
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Fri, 28 Apr 2023 13:13:57 GMT, Jan Lahoda wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 14 commits:
>>
>> - Merge branch 'master' into 8306112
>> - PreviewFeatures.isEnabled()
>> - Clean up isPreview
>> - Missing exception
>> - Corrections
>> - Update VM.java
>> - Clean up testing
>> - Update TestJavacTaskScanner.java
>> - Merge branch 'master' into 8306112
>> - Clean up
>> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 627:
>
>> 625: }
>> 626:
>> 627: public boolean isAnonymousMainClass() {
>
> This method seems a bit confusing to me. I believe the fields and methods
> will be stripped from `defs` if/when the wrapping class is created, which
> will mean this method will start to return `false`, no? It overall does not
> seem like a generally useful predicate.
>
> (If I understand this correctly, if we created the wrapping class in parser
> neither of these two methods would be needed.)
Inlining
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 640:
>
>> 638: // Find anonymous main class.
>> 639: for (JCTree def : defs) {
>> 640: if (def.hasTag(CLASSDEF)) {
>
> Nit, in cases like this, we lately tend to write `def instanceof JCClassDecl
> cls`, although we understand this way to check the AST is only safe inside
> javac.
Changing
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180491264
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180491108
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into 8306112
> - PreviewFeatures.isEnabled()
> - Clean up isPreview
> - Missing exception
> - Corrections
> - Update VM.java
> - Clean up testing
> - Update TestJavacTaskScanner.java
> - Merge branch 'master' into 8306112
> - Clean up
> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
3933:
> 3931: if
> (Feature.ANONYMOUS_MAIN_CLASSES.allowedInSource(source) &&
> 3932: !isDeclaration() &&
> 3933: (token.kind == VOID || token.kind ==
> IDENTIFIER)) {
These checks (`token.kind == VOID || token.kind == IDENTIFIER`) for token kind
will fail to parse fields of primitive types (or methods with primitive types
as a return type)? Either the tests must include the primitive types, or
(maybe) just this test can be avoided completely?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180481474
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Fri, 28 Apr 2023 12:45:35 GMT, Jan Lahoda wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 14 commits:
>>
>> - Merge branch 'master' into 8306112
>> - PreviewFeatures.isEnabled()
>> - Clean up isPreview
>> - Missing exception
>> - Corrections
>> - Update VM.java
>> - Clean up testing
>> - Update TestJavacTaskScanner.java
>> - Merge branch 'master' into 8306112
>> - Clean up
>> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
>
> src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 64:
>
>> 62:
>> 63: for (Method method : refc.getDeclaredMethods()) {
>> 64: int argc = method.getParameterCount();
>
> Nit: unused variable?
Remnant.
> src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 147:
>
>> 145: }
>> 146:
>> 147: return mainClass.getMethod("main", String[].class);
>
> Nit: could return `mainMethod` here, correct?
Yes
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 453:
>
>> 451: }
>> 452: if (!SourceVersion.isIdentifier(simplename) ||
>> SourceVersion.isKeyword(simplename)) {
>> 453: log.error(null, Errors.BadFileName(simplename));
>
> Suggestion:
>
> log.error(tree.pos(), Errors.BadFileName(simplename));
Changing
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 462:
>
>> 460: for (JCTree def : tree.defs) {
>> 461: if (def.hasTag(Tag.PACKAGEDEF)) {
>> 462: log.error(null,
>> Errors.AnonymousMainClassShouldNotHavePackageDeclaration);
>
> Suggestion:
>
> log.error(def.pos(),
> Errors.AnonymousMainClassShouldNotHavePackageDeclaration);
Changing
> src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line
> 438:
>
>> 436: Class appClass = Class.forName(mainClassName, true, cl);
>> 437: Method main = MainMethodFinder.findMainMethod(appClass);
>> 438: if (!PreviewFeatures.isEnabled() && (!isStatic(main) ||
>> !isPublic(main))) {
>
> It seems one can run a main method without parameters without
> `--enable-preview` using this codepath. That is presumably not intended.
MainMethodFinder won't return a no arg method is without --enable-preview. The
local testing here is to specialize the error massages.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180477997
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180477664
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180482962
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180482213
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180479992
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into 8306112
> - PreviewFeatures.isEnabled()
> - Clean up isPreview
> - Missing exception
> - Corrections
> - Update VM.java
> - Clean up testing
> - Update TestJavacTaskScanner.java
> - Merge branch 'master' into 8306112
> - Clean up
> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 64:
> 62:
> 63: for (Method method : refc.getDeclaredMethods()) {
> 64: int argc = method.getParameterCount();
Nit: unused variable?
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 147:
> 145: }
> 146:
> 147: return mainClass.getMethod("main", String[].class);
Nit: could return `mainMethod` here, correct?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 453:
> 451: }
> 452: if (!SourceVersion.isIdentifier(simplename) ||
> SourceVersion.isKeyword(simplename)) {
> 453: log.error(null, Errors.BadFileName(simplename));
Suggestion:
log.error(tree.pos(), Errors.BadFileName(simplename));
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 462:
> 460: for (JCTree def : tree.defs) {
> 461: if (def.hasTag(Tag.PACKAGEDEF)) {
> 462: log.error(null,
> Errors.AnonymousMainClassShouldNotHavePackageDeclaration);
Suggestion:
log.error(def.pos(),
Errors.AnonymousMainClassShouldNotHavePackageDeclaration);
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 438:
> 436: Class appClass = Class.forName(mainClassName, true, cl);
> 437: Method main = MainMethodFinder.findMainMethod(appClass);
> 438: if (!PreviewFeatures.isEnabled() && (!isStatic(main) ||
> !isPublic(main))) {
It seems one can run a main method without parameters without
`--enable-preview` using this codepath. That is presumably not intended.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 627:
> 625: }
> 626:
> 627: public boolean isAnonymousMainClass() {
This method seems a bit confusing to me. I believe the fields and methods will
be stripped from `defs` if/when the wrapping class is created, which will mean
this method will start to return `false`, no? It overall does not seem like a
generally useful predicate.
(If I understand this correctly, if we created the wrapping class in parser
neither of these two methods would be needed.)
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 640:
> 638: // Find anonymous main class.
> 639: for (JCTree def : defs) {
> 640: if (def.hasTag(CLASSDEF)) {
Nit, in cases like this, we lately tend to write `def instanceof JCClassDecl
cls`, although we understand this way to check the AST is only safe inside
javac.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180360702
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180360546
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180378753
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180378611
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180375691
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180389813
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180392123
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 20:34:44 GMT, Maurizio Cimadamore wrote: >> The source file name is not available until after parsing. > > I see that logic in JavaCompiler - I wonder if that's just the way it is, or > if there's a deeper reason as to why the sourcefile is set on the toplevel > unit *after* parsing (I don't think I can see any, in which case that might > be changed if that makes the rest of the code simpler). @lahodaj what do you > think? I believe we were discussing this some time ago, and there were some problems. I don't recall the exact details, but I'll try to look into this later. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180377062
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey wrote: >> Add flexible main methods and anonymous main classes to the Java language. > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 14 commits: > > - Merge branch 'master' into 8306112 > - PreviewFeatures.isEnabled() > - Clean up isPreview > - Missing exception > - Corrections > - Update VM.java > - Clean up testing > - Update TestJavacTaskScanner.java > - Merge branch 'master' into 8306112 > - Clean up > - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d I don't see anything hotspot related here (no idea why the auto-mapping added it) so can we remove hotspot-runtime please. - PR Comment: https://git.openjdk.org/jdk/pull/13689#issuecomment-1526968745
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into 8306112
> - PreviewFeatures.isEnabled()
> - Clean up isPreview
> - Missing exception
> - Corrections
> - Update VM.java
> - Clean up testing
> - Update TestJavacTaskScanner.java
> - Merge branch 'master' into 8306112
> - Clean up
> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
test/jdk/tools/launcher/OnrampMainTest.java line 31:
> 29: * @run main OnrampMainTest
> 30: */
> 31: public class OnrampMainTest extends TestHelper {
Should more tests be added for inherited methods?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179666410
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 18:28:41 GMT, Jim Laskey wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 432: >> >>> 430: >>> 431: // Restructure top level to be an top level anonymous class. >>> 432: public static void constructAnonymousMainClass(JCCompilationUnit >>> tree, >> >> Question: any reason as to why this is done here and not in the parser? >> Typically we don't want to do tree transformation at parse time, as that >> messes up clients that want to access the "non-desugared" tree (such as IDE) >> and expect some mapping between source to AST. But if you do the rewriting >> in Enter, not much changes, that is, clients such as IDEs would still see >> something that doesn't resemble the source. > > The source file name is not available until after parsing. I see that logic in JavaCompiler - I wonder if that's just the way it is, or if there's a deeper reason as to why the sourcefile is set on the toplevel unit *after* parsing (I don't think I can see any, in which case that might be changed if that makes the rest of the code simpler). @lahodaj what do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179650628
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 14:53:21 GMT, Maurizio Cimadamore wrote: >> Jim Laskey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> PreviewFeatures.isEnabled() > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 432: > >> 430: >> 431: // Restructure top level to be an top level anonymous class. >> 432: public static void constructAnonymousMainClass(JCCompilationUnit >> tree, > > Question: any reason as to why this is done here and not in the parser? > Typically we don't want to do tree transformation at parse time, as that > messes up clients that want to access the "non-desugared" tree (such as IDE) > and expect some mapping between source to AST. But if you do the rewriting in > Enter, not much changes, that is, clients such as IDEs would still see > something that doesn't resemble the source. The source file name is not available until after parsing. > src/jdk.compiler/share/classes/com/sun/tools/javac/parser/VirtualParser.java > line 191: > >> 189: * @param return type of parserAction >> 190: */ >> 191: public static Optional speculativeParse(JavacParser parser, > > Since this is never use, and because of the TODO - should we just drop it for > the time being? dropping - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179543180 PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179534229
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 14:55:01 GMT, Maurizio Cimadamore
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> PreviewFeatures.isEnabled()
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line
> 2433:
>
>> 2431: if (kind.contains(KindSelector.TYP)) {
>> 2432: sym = findType(env, name);
>> 2433: if ((sym.flags() & ANONYMOUS_MAIN_CLASS) != 0) {
>
> If `sym` is also `SYNTHTIC` (which it is) - do you need this? In what case?
> E.g. if there's a legitimate case for this I'm wondering if we should just
> test for `SYNTHETIC` rather than `ANONYMOUS_MAIN_CLASS`.
Yes, this is a remnant from earlier work.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179530497
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 13:39:37 GMT, Alan Bateman wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> PreviewFeatures.isEnabled()
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 45:
>
>> 43: import java.lang.invoke.MethodHandles;
>> 44: import java.lang.invoke.MethodHandles.Lookup;
>> 45: import java.lang.invoke.MethodType;
>
> Left over from an earlier version?
Yep.
> src/java.base/share/classes/sun/launcher/resources/launcher.properties line
> 240:
>
>> 238: Caused by: {1}: {2}
>> 239: java.launcher.cls.error8=\
>> 240: Error: no non-private no argument constructor found in class {0}
>
> Since this feature is for new developers then the error messages will need to
> be understand, maybe it should be "zero argument" and give an example to help.
java.launcher.cls.error8=\
Error: no non-private zero argument constructor found in class {0}\n\
remove private from existing constructor or define as:\n\
\ public {0}() {}
> src/java.base/share/native/libjli/java.c line 590:
>
>> 588: CHECK_EXCEPTION_NULL_LEAVE(mainID);
>> 589: (*env)->CallVoidMethod(env, mainObject, mainID);
>> 590: break;
>
> This calls into LauncherHelper to get the "main type", then calls the static
> or new/instance method. I'm wondering if you tried have a single entry point
> in LauncherHelper instead. I think the only downside might be that the
> trampoline would show up in stack traces but @Hidden could hide that.
Good idea. If @hidden doesn't work then we can eat the trace entries.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179518999
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179528956
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179517981
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Merge branch 'master' into 8306112 - PreviewFeatures.isEnabled() - Clean up isPreview - Missing exception - Corrections - Update VM.java - Clean up testing - Update TestJavacTaskScanner.java - Merge branch 'master' into 8306112 - Clean up - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d - Changes: https://git.openjdk.org/jdk/pull/13689/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13689=02 Stats: 1240 lines in 29 files changed: 1086 ins; 62 del; 92 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 13:17:58 GMT, Jim Laskey wrote: >> Add flexible main methods and anonymous main classes to the Java language. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > PreviewFeatures.isEnabled() I assume future iterations of the PR will include handling and tests for the main class attribute of jar files. - PR Comment: https://git.openjdk.org/jdk/pull/13689#issuecomment-1525943897
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 13:17:58 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> PreviewFeatures.isEnabled()
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 432:
> 430:
> 431: // Restructure top level to be an top level anonymous class.
> 432: public static void constructAnonymousMainClass(JCCompilationUnit
> tree,
Question: any reason as to why this is done here and not in the parser?
Typically we don't want to do tree transformation at parse time, as that messes
up clients that want to access the "non-desugared" tree (such as IDE) and
expect some mapping between source to AST. But if you do the rewriting in
Enter, not much changes, that is, clients such as IDEs would still see
something that doesn't resemble the source.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 2433:
> 2431: if (kind.contains(KindSelector.TYP)) {
> 2432: sym = findType(env, name);
> 2433: if ((sym.flags() & ANONYMOUS_MAIN_CLASS) != 0) {
If `sym` is also `SYNTHTIC` (which it is) - do you need this? In what case?
E.g. if there's a legitimate case for this I'm wondering if we should just test
for `SYNTHETIC` rather than `ANONYMOUS_MAIN_CLASS`.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/VirtualParser.java
line 191:
> 189: * @param return type of parserAction
> 190: */
> 191: public static Optional speculativeParse(JavacParser parser,
Since this is never use, and because of the TODO - should we just drop it for
the time being?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179286118
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179288860
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179292509
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 13:17:58 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> PreviewFeatures.isEnabled()
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 45:
> 43: import java.lang.invoke.MethodHandles;
> 44: import java.lang.invoke.MethodHandles.Lookup;
> 45: import java.lang.invoke.MethodType;
Left over from an earlier version?
src/java.base/share/classes/sun/launcher/resources/launcher.properties line 240:
> 238: Caused by: {1}: {2}
> 239: java.launcher.cls.error8=\
> 240: Error: no non-private no argument constructor found in class {0}
Since this feature is for new developers then the error messages will need to
be understand, maybe it should be "zero argument" and give an example to help.
src/java.base/share/native/libjli/java.c line 590:
> 588: CHECK_EXCEPTION_NULL_LEAVE(mainID);
> 589: (*env)->CallVoidMethod(env, mainObject, mainID);
> 590: break;
This calls into LauncherHelper to get the "main type", then calls the static or
new/instance method. I'm wondering if you tried have a single entry point in
LauncherHelper instead. I think the only downside might be that the trampoline
would show up in stack traces but @Hidden could hide that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179174808
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179183719
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1179172679
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: PreviewFeatures.isEnabled() - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/cfe08f33..a55af76c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=00-01 Stats: 32 lines in 3 files changed: 2 ins; 27 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview)
Add flexible main methods and anonymous main classes to the Java language. - Commit messages: - Clean up isPreview - Missing exception - Corrections - Update VM.java - Clean up testing - Update TestJavacTaskScanner.java - Merge branch 'master' into 8306112 - Clean up - remnant import - Revised main method lookup - ... and 2 more: https://git.openjdk.org/jdk/compare/98e8616a...cfe08f33 Changes: https://git.openjdk.org/jdk/pull/13689/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13689=00 Issue: https://bugs.openjdk.org/browse/JDK-8306112 Stats: 1265 lines in 29 files changed: ins; 62 del; 92 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
