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
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo <prappo@openjdk.org> 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<Locale> 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<Locale> JAVA_BASE_LOCALES
Canonical modifier order is `static final` ------------- Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6191
On Mon, 1 Nov 2021 15:23:49 GMT, Claes Redestad <redestad@openjdk.org> 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<Locale> 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
On Mon, 1 Nov 2021 16:25:26 GMT, Pavel Rappo <prappo@openjdk.org> 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<Locale> 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
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo <prappo@openjdk.org> 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<Locale> 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
On Mon, 1 Nov 2021 15:52:51 GMT, Daniel Fuchs <dfuchs@openjdk.org> 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<Locale> 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... So the answer is yes. ------------- PR: https://git.openjdk.java.net/jdk/pull/6191
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo <prappo@openjdk.org> 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
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
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo <prappo@openjdk.org> 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
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo <prappo@openjdk.org> 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
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo <prappo@openjdk.org> wrote:
Please review this PR. A comprehensive test job has been scheduled; I'll notify this thread once that job has completed.
This pull request has now been integrated. Changeset: 2eafa036 Author: Pavel Rappo <prappo@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/2eafa036c03d3e8f3dba8f67dd37b484874d... Stats: 13 lines in 3 files changed: 0 ins; 1 del; 12 mod 8276234: Trivially clean up locale-related code Reviewed-by: redestad, naoto, iris ------------- PR: https://git.openjdk.java.net/jdk/pull/6191
participants (5)
-
Claes Redestad
-
Daniel Fuchs
-
Iris Clark
-
Naoto Sato
-
Pavel Rappo