Re: RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option [v2]
On Fri, 14 Jul 2023 11:21:02 GMT, Sean Coffey wrote: >> Simple tweak to remove the "available locales" section from default >> `-XshowSettings` output. >> >> Instead, it remains available with the `-XshowSettings:locale` option > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback and bug id added to test thanks for the review @RogerRiggs - nice ideas but I'm not sure if it makes the code any clearer. I've implemented your suggestions but I feel we're polluting the main method with extra printlns and flow logic for the sake of locale output. Placing the help message option inside the "locale settings summary" style header works better also IMO. I think your suggestion means we print the helpful "more info with :locale " hint in the main showSettings call also, again pollution of main method. Best to let the flow of the locale setting logic stem from a printLocale(boolean summary) type method IMO - PR Comment: https://git.openjdk.org/jdk/pull/14885#issuecomment-1636060048
Re: RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option [v2]
On Fri, 14 Jul 2023 11:21:02 GMT, Sean Coffey wrote:
>> Simple tweak to remove the "available locales" section from default
>> `-XshowSettings` output.
>>
>> Instead, it remains available with the `-XshowSettings:locale` option
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback and bug id added to test
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 173:
> 171: break;
> 172: case "locale":
> 173: printLocale(false);
Adding the flag seems to make the coding more complex.
I'd remove the printing of all locales from the printLocale method (and maybe
rename it to printLocalSummary).
And add the printing of the help test for the the verbose here also.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 187:
> 185: printVmSettings(initialHeapSize, maxHeapSize, stackSize);
> 186: printProperties();
> 187: printLocale(true);
Here: call `printLocaleSummary()` and then `printLocales()` and println.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 283:
> 281: * prints the locale subopt/section
> 282: */
> 283: private static void printLocale(boolean summaryMode) {
Rename to printLocaleSummary (without an argument) and just print the summary.
(No help).
And drop the printing of the locales.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14885#discussion_r1263865162
PR Review Comment: https://git.openjdk.org/jdk/pull/14885#discussion_r1263865996
PR Review Comment: https://git.openjdk.org/jdk/pull/14885#discussion_r1263870691
Re: RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option [v2]
On Fri, 14 Jul 2023 11:21:02 GMT, Sean Coffey wrote: >> Simple tweak to remove the "available locales" section from default >> `-XshowSettings` output. >> >> Instead, it remains available with the `-XshowSettings:locale` option > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback and bug id added to test Thank you Sean for the update. Looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14885#pullrequestreview-1530122813
Re: RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option [v2]
On Fri, 14 Jul 2023 11:02:33 GMT, Jaikiran Pai wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review feedback and bug id added to test
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 289:
>
>> 287: } else {
>> 288: ostream.println("Locale settings summary:");
>> 289: ostream.println(INDENT + "Use \":locale\" " +
>
> Hello Sean, should this instead say "Use "-XshowSettings:locale" " so
> that it's clear that this is a sub-option for the -XshowSettings option?
Yes, no harm to be clearer Jai. Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14885#discussion_r1263614727
Re: RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option [v2]
> Simple tweak to remove the "available locales" section from default > `-XshowSettings` output. > > Instead, it remains available with the `-XshowSettings:locale` option Sean Coffey has updated the pull request incrementally with one additional commit since the last revision: review feedback and bug id added to test - Changes: - all: https://git.openjdk.org/jdk/pull/14885/files - new: https://git.openjdk.org/jdk/pull/14885/files/0366dcc5..2aeec368 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14885&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14885&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14885.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14885/head:pull/14885 PR: https://git.openjdk.org/jdk/pull/14885
Re: RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option
On Fri, 14 Jul 2023 10:01:16 GMT, Sean Coffey wrote:
> Simple tweak to remove the "available locales" section from default
> `-XshowSettings` output.
>
> Instead, it remains available with the `-XshowSettings:locale` option
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 289:
> 287: } else {
> 288: ostream.println("Locale settings summary:");
> 289: ostream.println(INDENT + "Use \":locale\" " +
Hello Sean, should this instead say "Use "-XshowSettings:locale" " so that
it's clear that this is a sub-option for the -XshowSettings option?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14885#discussion_r1263602411
RFR: 8310201: Reduce verbose locale output in -XshowSettings launcher option
Simple tweak to remove the "available locales" section from default `-XshowSettings` output. Instead, it remains available with the `-XshowSettings:locale` option - Commit messages: - 8310201 Changes: https://git.openjdk.org/jdk/pull/14885/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14885&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310201 Stats: 21 lines in 2 files changed: 15 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14885.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14885/head:pull/14885 PR: https://git.openjdk.org/jdk/pull/14885
