On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey <jlas...@openjdk.org> 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
