RFR: 8276234: Trivially clean up locale-related code
Please review this PR. A comprehensive test job has been scheduled; I'll notify this thread once that job has completed. - Commit messages: - Fix *code* typo - Be more immutable - Fix typos Changes: https://git.openjdk.java.net/jdk/pull/6191/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6191&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276234 Stats: 10 lines in 3 files changed: 0 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/6191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6191/head:pull/6191 PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote: > Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. LGTM, but please use `static final` src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: > 246: private static final LocaleDataStrategy INSTANCE = new > LocaleDataStrategy(); > 247: // TODO: avoid hard-coded Locales > 248: private final static Set JAVA_BASE_LOCALES Canonical modifier order is `static final` src/java.base/share/classes/sun/util/resources/LocaleData.java line 327: > 325: = new SupplementaryStrategy(); > 326: // TODO: avoid hard-coded Locales > 327: private final static Set JAVA_BASE_LOCALES Canonical modifier order is `static final` - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote:
> Please review this PR. A comprehensive test job has been scheduled; I'll
> notify this thread once that job has completed.
src/java.base/share/classes/sun/util/resources/LocaleData.java line 336:
> 334: public List getCandidateLocales(String baseName, Locale
> locale) {
> 335: // Specify only the given locale
> 336: return List.of(locale);
Is it guaranteed that `locale` is not `null` here?
-
PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote: > Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:52:51 GMT, Daniel Fuchs wrote:
>> Please review this PR. A comprehensive test job has been scheduled; I'll
>> notify this thread once that job has completed.
>
> src/java.base/share/classes/sun/util/resources/LocaleData.java line 336:
>
>> 334: public List getCandidateLocales(String baseName, Locale
>> locale) {
>> 335: // Specify only the given locale
>> 336: return List.of(locale);
>
> Is it guaranteed that `locale` is not `null` here?
This is a good question. As far I can see, the only call site checks for null
explicitly:
https://github.com/openjdk/jdk/blob/a4c46e1e4f4f2f05c8002b2af683a390fc46b424/src/java.base/share/classes/sun/util/resources/Bundles.java#L113
So the answer is yes.
-
PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:23:49 GMT, Claes Redestad wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: > >> 246: private static final LocaleDataStrategy INSTANCE = new >> LocaleDataStrategy(); >> 247: // TODO: avoid hard-coded Locales >> 248: private final static Set JAVA_BASE_LOCALES > > Canonical modifier order is `static final` It turns out that my IDE was configured NOT to highlight missorted modifiers. Once I reconfigured it, I saw these cases and some other in that same file. I'll fix them all if that's okay. My recollection is that there have been campaigns on using "the blessed modifiers order". It might be that since the last such crusade the modifiers have gone out of hand, and we might need to re-bless them :-) - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:25:26 GMT, Pavel Rappo wrote: >> src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: >> >>> 246: private static final LocaleDataStrategy INSTANCE = new >>> LocaleDataStrategy(); >>> 247: // TODO: avoid hard-coded Locales >>> 248: private final static Set JAVA_BASE_LOCALES >> >> Canonical modifier order is `static final` > > It turns out that my IDE was configured NOT to highlight missorted modifiers. > Once I reconfigured it, I saw these cases and some other in that same file. > I'll fix them all if that's okay. > > My recollection is that there have been campaigns on using "the blessed > modifiers order". It might be that since the last such crusade the modifiers > have gone out of hand, and we might need to re-bless them :-) Fixed in 2b7e0c6. - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
> Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Use the blessed modifiers order - Changes: - all: https://git.openjdk.java.net/jdk/pull/6191/files - new: https://git.openjdk.java.net/jdk/pull/6191/files/8e2815b8..2b7e0c68 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6191&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6191&range=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6191/head:pull/6191 PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Use the blessed modifiers order Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Use the blessed modifiers order The test job that has been scheduled previously has completed successfully. - PR: https://git.openjdk.java.net/jdk/pull/6191
