On Fri, 5 May 2023 09:52:37 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 872:
>
>> 870: Method mainMethod = null;
>> 871: try {
>> 872: mainMethod = MainMethodFinder.findMainMethod(mainClass);
>
> The `MainMethodFinder.findMainMethod(...)` throws a `NoSuchMethodException`
> when it can't find the regular `public static void main(String[])` or, when
> preview is enabled, any other eligible main methods. That will now mean that
> the next line here which catches a `NoSuchMethodException` can potentially
> end up aborting with a (very confusing) error message about JavaFX
> application. We should perhaps change that catch block to something like:
>
> } catch (NoSuchMethodException nsme) {
> if (!PreviewFeatures.isEnabled()) {
> // invalid main or not FX application, abort with an error
> abort(null, "java.launcher.cls.error4", mainClass.getName(),
> JAVAFX_APPLICATION_CLASS_NAME);
> } else {
> // abort with something more clear error?
> }
Not convinced that anything has changed or that the message is confusing.
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 893:
>
>> 891: * findMainMethod (above) will choose the correct method, based
>> 892: * on its name and parameter type, however, we still have to
>> 893: * ensure that the method is static (non-preview) and returns a
>> void.
>
> I think it's probably going to be easier to read and maintain if we moved
> these checks for `static` and `void` return type into the
> `MainMethodFinder.findMainMethod(...)` itself.
> What I mean is once we return from the `findMainMethod(...)`, I think the
> callers, like this code here, shouldn't have to do additional checks to know
> if this main method is valid and can be used.
I agree, but from my early commentary we have to hold off until later.
> src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line
> 419:
>
>> 417: private static boolean isStatic(Method method) {
>> 418: return method != null &&
>> Modifier.isStatic(method.getModifiers());
>> 419: }
>
> It looks like these new methods aren't being used.
Remnants - removing
> src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line
> 440:
>
>> 438: int mods = mainMethod.getModifiers();
>> 439: boolean isStatic = Modifier.isStatic(mods);
>> 440: boolean isPublic = Modifier.isStatic(mods);
>
> This looks like a typo. This should have been `Modifier.isPublic(mods);`
Changing
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186258290
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186259446
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186261107
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186259830
