Re: RFR: 8316557: Make fields final in 'sun.util' package [v3]
On Thu, 16 Nov 2023 18:11:48 GMT, Andrey Turbanov wrote: >> A few classes in `sun.util` package have non-final fields which could easily >> be marked `final`. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8316557: Make fields final in 'sun.util' package > > "First Day" is the first day of the week and should not be plural Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15736#pullrequestreview-1737870814
Re: RFR: 8316557: Make fields final in 'sun.util' package [v2]
On Fri, 17 Nov 2023 08:38:37 GMT, Andrey Turbanov wrote: >> 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 - PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1397697691
Re: RFR: 8316557: Make fields final in 'sun.util' package [v2]
On Fri, 17 Nov 2023 08:13:05 GMT, Per Minborg wrote:
>> src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java
>> line 48:
>>
>>> 46: public class CLDRCalendarDataProviderImpl extends
>>> CalendarDataProviderImpl {
>>> 47:
>>> 48: private static final Map 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1396868339
Re: RFR: 8316557: Make fields final in 'sun.util' package [v2]
On Thu, 16 Nov 2023 17:08:58 GMT, Naoto Sato wrote:
>> Andrey Turbanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8316557: Make fields final in 'sun.util' package
>>
>> rename 'firstDay' to 'firstDays' to match 'minDays' naming
>
> src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java
> line 48:
>
>> 46: public class CLDRCalendarDataProviderImpl extends
>> CalendarDataProviderImpl {
>> 47:
>> 48: private static final Map 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1396844057
Re: RFR: 8316557: Make fields final in 'sun.util' package [v3]
> A few classes in `sun.util` package have non-final fields which could easily > be marked `final`. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8316557: Make fields final in 'sun.util' package "First Day" is the first day of the week and should not be plural - Changes: - all: https://git.openjdk.org/jdk/pull/15736/files - new: https://git.openjdk.org/jdk/pull/15736/files/5db74105..2c9b4edc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15736=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15736=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15736.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15736/head:pull/15736 PR: https://git.openjdk.org/jdk/pull/15736
Re: RFR: 8316557: Make fields final in 'sun.util' package [v2]
On Thu, 16 Nov 2023 08:56:05 GMT, Andrey Turbanov wrote:
>> A few classes in `sun.util` package have non-final fields which could easily
>> be marked `final`.
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8316557: Make fields final in 'sun.util' package
>
> rename 'firstDay' to 'firstDays' to match 'minDays' naming
Changes requested by naoto (Reviewer).
src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java
line 48:
> 46: public class CLDRCalendarDataProviderImpl extends
> CalendarDataProviderImpl {
> 47:
> 48: private static final Map firstDays = new
> ConcurrentHashMap<>();
I don't think this is correct. "First Day" is the first day of the week and
should not be plural.
-
PR Review: https://git.openjdk.org/jdk/pull/15736#pullrequestreview-1734986993
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1396065609
Re: RFR: 8316557: Make fields final in 'sun.util' package [v2]
> A few classes in `sun.util` package have non-final fields which could easily > be marked `final`. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8316557: Make fields final in 'sun.util' package rename 'firstDay' to 'firstDays' to match 'minDays' naming - Changes: - all: https://git.openjdk.org/jdk/pull/15736/files - new: https://git.openjdk.org/jdk/pull/15736/files/26d00dcb..5db74105 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15736=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15736=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15736.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15736/head:pull/15736 PR: https://git.openjdk.org/jdk/pull/15736
Re: RFR: 8316557: Make fields final in 'sun.util' package [v2]
On Mon, 13 Nov 2023 11:40:27 GMT, Per Minborg wrote:
>> Andrey Turbanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8316557: Make fields final in 'sun.util' package
>>
>> rename 'firstDay' to 'firstDays' to match 'minDays' naming
>
> src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java
> line 48:
>
>> 46: public class CLDRCalendarDataProviderImpl extends
>> CalendarDataProviderImpl {
>> 47:
>> 48: private static final Map 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.
> src/java.base/share/classes/sun/util/locale/LocaleObjectCache.java line 102:
>
>> 100: }
>> 101:
>> 102: private static class CacheEntry extends SoftReference {
>
> The class itself might be `final`
I think making classes `final` is out of scope of this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1395350055
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1395351993
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Thu, 14 Sep 2023 08:58:56 GMT, Andrey Turbanov wrote:
> A few classes in `sun.util` package have non-final fields which could easily
> be marked `final`.
LGTM. See comments though.
src/java.base/share/classes/sun/util/locale/StringTokenIterator.java line 33:
> 31: package sun.util.locale;
> 32:
> 33: public class StringTokenIterator {
The class may be `final`
-
Marked as reviewed by pminborg (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15736#pullrequestreview-1727154051
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1391008427
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Thu, 14 Sep 2023 08:58:56 GMT, Andrey Turbanov wrote:
> A few classes in `sun.util` package have non-final fields which could easily
> be marked `final`.
src/java.base/share/classes/sun/util/cldr/CLDRCalendarDataProviderImpl.java
line 48:
> 46: public class CLDRCalendarDataProviderImpl extends
> CalendarDataProviderImpl {
> 47:
> 48: private static final Map firstDay = new
> ConcurrentHashMap<>();
We might rename these fields to FIRST_DAY etc.
src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java line
63:
> 61:
> 62: // parent locales map
> 63: private static final Map parentLocalesMap;
Rename to PARENT_LOCALES_MAP?
src/java.base/share/classes/sun/util/locale/LocaleObjectCache.java line 102:
> 100: }
> 101:
> 102: private static class CacheEntry extends SoftReference {
The class itself might be `final`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1391002631
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1391004183
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1391005224
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Thu, 14 Sep 2023 08:58:56 GMT, Andrey Turbanov wrote: > A few classes in `sun.util` package have non-final fields which could easily > be marked `final`. Can I get a review? - PR Comment: https://git.openjdk.org/jdk/pull/15736#issuecomment-1807066050
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Mon, 25 Sep 2023 10:04:13 GMT, Chen Liang wrote: >>> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1335672904
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Mon, 25 Sep 2023 09:53:47 GMT, Glavo wrote: >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1335672130
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Mon, 25 Sep 2023 09:47:15 GMT, Chen Liang wrote: > UTF8 decoder does not perform any internal state mutation during decoding; Are you sure? I think `CharsetDecoder::decode` will modify the `status` field. - PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1335660344
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Mon, 25 Sep 2023 09:40:00 GMT, Andrey Turbanov wrote:
>> src/java.base/share/classes/sun/util/PropertyResourceBundleCharset.java line
>> 71:
>>
>>> 69: private final class PropertiesFileDecoder extends CharsetDecoder {
>>> 70:
>>> 71: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1335653054
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Mon, 18 Sep 2023 14:41:14 GMT, Chen Liang wrote:
>> A few classes in `sun.util` package have non-final fields which could easily
>> be marked `final`.
>
> src/java.base/share/classes/sun/util/PropertyResourceBundleCharset.java line
> 71:
>
>> 69: private final class PropertiesFileDecoder extends CharsetDecoder {
>> 70:
>> 71: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1335644148
Re: RFR: 8316557: Make fields final in 'sun.util' package
On Thu, 14 Sep 2023 08:58:56 GMT, Andrey Turbanov wrote:
> A few classes in `sun.util` package have non-final fields which could easily
> be marked `final`.
src/java.base/share/classes/sun/util/PropertyResourceBundleCharset.java line 71:
> 69: private final class PropertiesFileDecoder extends CharsetDecoder {
> 70:
> 71: private final CharsetDecoder cdUTF_8 = UTF_8.INSTANCE.newDecoder()
Can be static as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15736#discussion_r1328836367
RFR: 8316557: Make fields final in 'sun.util' package
A few classes in `sun.util` package have non-final fields which could easily be marked `final`. - Commit messages: - [PATCH] Make fields final in 'sun.util' package Changes: https://git.openjdk.org/jdk/pull/15736/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15736=00 Issue: https://bugs.openjdk.org/browse/JDK-8316557 Stats: 42 lines in 10 files changed: 2 ins; 13 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/15736.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15736/head:pull/15736 PR: https://git.openjdk.org/jdk/pull/15736
