8316557: Make fields final in 'sun.util' package #15736

turbanoff · 2023-09-14T08:58:56Z

A few classes in sun.util package have non-final fields which could easily be marked final.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8316557: Make fields final in 'sun.util' package (Enhancement - P5)

Reviewers

Per Minborg (@minborg - Reviewer)
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15736/head:pull/15736
$ git checkout pull/15736

Update a local copy of the PR:
$ git checkout pull/15736
$ git pull https://git.openjdk.org/jdk.git pull/15736/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15736

View PR using the GUI difftool:
$ git pr show -t 15736

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15736.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-14T08:59:53Z

👋 Welcome back aturbanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-14T09:01:28Z

@turbanoff The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

liach · 2023-09-18T14:41:14Z

src/java.base/share/classes/sun/util/PropertyResourceBundleCharset.java

@@ -73,7 +68,7 @@ public CharsetEncoder newEncoder() {

    private final class PropertiesFileDecoder extends CharsetDecoder {

-        private CharsetDecoder cdUTF_8 = UTF_8.INSTANCE.newDecoder()
+        private final CharsetDecoder cdUTF_8 = UTF_8.INSTANCE.newDecoder()


Can be static as well.

I'm not sure. Why do you think so? CharsetDecoder is not thread-safe.

UTF8 decoder does not perform any internal state mutation during decoding; in addition, if this field is static final, the decoder's error actions are published when the class is initialized, while publishing in a final field does not guarantee the internal state of the charset is visible to other threads.

UTF8 decoder does not perform any internal state mutation during decoding;

Are you sure? I think CharsetDecoder::decode will modify the status field.

right, it does have a state field.

CharsetDecoder is specified to be not safe for use by multiple concurrent threads, would be too fragile to assume behavior of a specific implementation.

mlbridge · 2023-09-19T20:22:04Z

Webrevs

bridgekeeper · 2023-10-23T13:37:22Z

@turbanoff This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

turbanoff · 2023-11-12T09:14:50Z

Can I get a review?

minborg · 2023-11-13T11:40:27Z

src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java

@@ -47,8 +45,8 @@
 */
 public class CLDRCalendarDataProviderImpl extends CalendarDataProviderImpl {

-    private static Map<String, Integer> firstDay = new ConcurrentHashMap<>();
-    private static Map<String, Integer> minDays = new ConcurrentHashMap<>();
+    private static final Map<String, Integer> firstDay = new ConcurrentHashMap<>();


We might rename these fields to FIRST_DAY etc.

Hm. Usually CAPS naming is used only on immutable constants, but here we have mutable field. (See google style guide for example - https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names).
I like old naming approach.

minborg · 2023-11-13T11:41:27Z

src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java

@@ -60,7 +60,7 @@ public class CLDRLocaleProviderAdapter extends JRELocaleProviderAdapter {
    private final LocaleDataMetaInfo nonBaseMetaInfo;

    // parent locales map
-    private static volatile Map<Locale, Locale> parentLocalesMap;
+    private static final Map<Locale, Locale> parentLocalesMap;


Rename to PARENT_LOCALES_MAP?

minborg · 2023-11-13T11:42:33Z

src/java.base/share/classes/sun/util/locale/LocaleObjectCache.java

@@ -100,7 +100,7 @@ protected K normalizeKey(K key) {
    }

    private static class CacheEntry<K, V> extends SoftReference<V> {


The class itself might be final

I think making classes final is out of scope of this PR.

minborg · 2023-11-13T11:45:57Z

src/java.base/share/classes/sun/util/locale/StringTokenIterator.java

@@ -31,9 +31,9 @@
 package sun.util.locale;

 public class StringTokenIterator {


The class may be final

minborg

LGTM. See comments though.

openjdk · 2023-11-13T11:49:57Z

@turbanoff This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8316557: Make fields final in 'sun.util' package

Reviewed-by: pminborg, naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 862 new commits pushed to the master branch:

0881f2b: 8318562: Computational test more than 2x slower when AVX instructions are used
9b372e2: 8320206: Some intrinsics/stubs missing vzeroupper on x86_64
b5a7562: 8319985: Delete sun.awt.windows.WToolkit.embedded*() API
1fce70b: 8320334: Reflow markdown in testing.md
9902d2e: 8315457: Implement JEP 459: String Templates (Second Preview)
5522656: 8317834: java/lang/Thread/IsAlive.java timed out
9194d2c: 8317357: Update links in building.md to use https rather than http
368e4f6: 8315801: [PPC64] JNI code should be more similar to the Panama implementation
8ec6b8d: 8319876: Reduce memory consumption of VM_ThreadDump::doit
bbf52e0: 8319897: Move StackWatermark handling out of LockStack::contains
... and 852 more: https://git.openjdk.org/jdk/compare/903b9e8dd966fbb61222c59048b752ed8b42b608...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

rename 'firstDay' to 'firstDays' to match 'minDays' naming

naotoj · 2023-11-16T17:08:58Z

src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java

@@ -47,8 +45,8 @@
 */
 public class CLDRCalendarDataProviderImpl extends CalendarDataProviderImpl {

-    private static Map<String, Integer> firstDay = new ConcurrentHashMap<>();
-    private static Map<String, Integer> minDays = new ConcurrentHashMap<>();
+    private static final Map<String, Integer> firstDays = new ConcurrentHashMap<>();


I don't think this is correct. "First Day" is the first day of the week and should not be plural.

The map contains several entries so I think the plural form is appropriate here.

Current naming highlights the difference in values in this maps: FIRST_DAY_OF_WEEK vs MINIMAL_DAYS_IN_FIRST_WEEK.
I like this approach.

Maybe firstDayMap/minDaysMap would clarify more, but either way is fine with me

"First Day" is the first day of the week and should not be plural

turbanoff · 2023-11-20T09:40:58Z

/integrate

openjdk · 2023-11-20T09:41:42Z

Going to push as commit 6c5e15c.
Since your change was applied there have been 867 commits pushed to the master branch:

de51aa1: 8283214: [macos] Screen magnifier does not show the magnified text for JcomboBox
179f505: 8320361: Doc error in RemoteRecordingStream.java
8ff7d6e: 8295391: Add discussion of binary <-> decimal conversion issues
99570fb: 8320258: Refresh building.md
a1e7a30: 8320212: Disable GCC stringop-overflow warning for affected files
0881f2b: 8318562: Computational test more than 2x slower when AVX instructions are used
9b372e2: 8320206: Some intrinsics/stubs missing vzeroupper on x86_64
b5a7562: 8319985: Delete sun.awt.windows.WToolkit.embedded*() API
1fce70b: 8320334: Reflow markdown in testing.md
9902d2e: 8315457: Implement JEP 459: String Templates (Second Preview)
... and 857 more: https://git.openjdk.org/jdk/compare/903b9e8dd966fbb61222c59048b752ed8b42b608...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-20T09:41:49Z

@turbanoff Pushed as commit 6c5e15c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[PATCH] Make fields final in 'sun.util' package

26d00dc

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Sep 14, 2023

liach reviewed Sep 18, 2023

View reviewed changes

turbanoff changed the title ~~[PATCH] Make fields final in 'sun.util' package~~ 8316557: Make fields final in 'sun.util' package Sep 19, 2023

openjdk bot added the rfr Pull request is ready for review label Sep 19, 2023

minborg reviewed Nov 13, 2023

View reviewed changes

minborg approved these changes Nov 13, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 13, 2023

8316557: Make fields final in 'sun.util' package

5db7410

rename 'firstDay' to 'firstDays' to match 'minDays' naming

naotoj suggested changes Nov 16, 2023

View reviewed changes

8316557: Make fields final in 'sun.util' package

2c9b4ed

"First Day" is the first day of the week and should not be plural

turbanoff requested a review from naotoj November 17, 2023 08:27

naotoj approved these changes Nov 17, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 20, 2023

openjdk bot closed this Nov 20, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8316557: Make fields final in 'sun.util' package #15736

8316557: Make fields final in 'sun.util' package #15736

turbanoff commented Sep 14, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 14, 2023

openjdk bot commented Sep 14, 2023

liach Sep 18, 2023

turbanoff Sep 25, 2023

liach Sep 25, 2023

Glavo Sep 25, 2023

liach Sep 25, 2023

AlanBateman Sep 25, 2023

mlbridge bot commented Sep 19, 2023 •

edited

Loading

bridgekeeper bot commented Oct 23, 2023

turbanoff commented Nov 12, 2023

minborg Nov 13, 2023

turbanoff Nov 16, 2023

minborg Nov 13, 2023

minborg Nov 13, 2023

turbanoff Nov 16, 2023

minborg Nov 13, 2023

minborg left a comment

openjdk bot commented Nov 13, 2023 •

edited

Loading

naotoj Nov 16, 2023

minborg Nov 17, 2023

turbanoff Nov 17, 2023

naotoj Nov 17, 2023

turbanoff commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

		@@ -100,7 +100,7 @@ protected K normalizeKey(K key) {
		}

		private static class CacheEntry<K, V> extends SoftReference<V> {

		@@ -31,9 +31,9 @@
		package sun.util.locale;

		public class StringTokenIterator {

8316557: Make fields final in 'sun.util' package #15736

8316557: Make fields final in 'sun.util' package #15736

Conversation

turbanoff commented Sep 14, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 14, 2023

openjdk bot commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Sep 19, 2023 • edited Loading

Webrevs

bridgekeeper bot commented Oct 23, 2023

turbanoff commented Nov 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minborg left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbanoff commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

turbanoff commented Sep 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 19, 2023 •

edited

Loading

openjdk bot commented Nov 13, 2023 •

edited

Loading