On Thu, 11 May 2023 11:25:51 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 incrementally with one additional
> commit since the last revision:
>
> Update VirtualParser.java
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 35:
> 33: public class MainMethodFinder {
> 34: private static boolean isPrivate(Method method) {
> 35: return method != null &&
> Modifier.isPrivate(method.getModifiers());
Are you sure you want to allow null here? It seems like it's a bug in the
caller if that happens.
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 38:
> 36: }
> 37:
> 38: private static boolean isPublic(Method method) {
Is this left over from a previous iteration, it doesn't seem to be used.
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 53:
> 51:
> 52: /**
> 53: * Gather all the "main" methods in the class heirarchy.
heirarchy -> hierarchy
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 134:
> 132:
> 133: /**
> 134: * {@return priority main method or null if none found}
"or null if none found", is that out of date?
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 146:
> 144:
> 145: if (Modifier.isStatic(mods) &&
> mainMethod.getDeclaringClass() != mainClass) {
> 146: System.err.println("WARNING: static main in super class
> will be deprecated.");
I thought that JEP 445 was deprecating this, in which case the text should be
"is deprecated" rather than "will be".
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 156:
> 154:
> 155: List<Method> mains = new ArrayList<>();
> 156: gatherMains(mainClass, mainClass, mains);
Instead of gatherMains, did you consider first looking for static
main(String[], then static main()? Asking because I expected to only see the
walk up the hierarchy when looking for an instance main.
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 162:
> 160: }
> 161:
> 162: if (1 < mains.size()) {
Checking if mains.size() > 1 might be easier on the eyes.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 872:
> 870:
> 871: // Check the existence and signature of main and abort if incorrect
> 872: public static void validateMainClass(Class<?> mainClass) {
Is there a reason that this is changed to public, maybe left over from a
previous iteration?
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 896:
> 894: * findMainMethod (above) will choose the correct method, based
> 895: * on its name and parameter type, however, we still have to
> 896: * ensure that the method is static (non-preview) and returns a
> void.
Have you looked into findMainMethod checking the return type? Right now, we
have findMainMethod returning a Method that needs further checking.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 904:
> 902:
> 903: if (!PreviewFeatures.isEnabled()) {
> 904: if (!isStatic || !isPublic || noArgs) {
You can use && here and avoid the nested if.
test/jdk/tools/launcher/InstanceMainTest.java line 31:
> 29: * @run main InstanceMainTest
> 30: */
> 31: public class InstanceMainTest extends TestHelper {
Are you planning to add tests for the selection/precedence order?
Also wondering if we need a test to check that there is a warning when the main
method in found in the super class.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193379090
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193378248
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193379468
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193415367
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193385008
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193440438
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193419331
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193444639
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193455653
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193443762
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193390566
