Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v5]
On Fri, 15 Sep 2023 20:24:35 GMT, Justin Lu wrote: >> Please review this PR which is a continuation of >> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused >> code from the _sun.util.Calendar_ classes. >> >> `forceStandardTime` is always false. >> >> In addition, `locale` is never by used by _CalendarDate_ or any inheritors >> and can be removed. >> >> As a result, _ImmutableGregorianDate_ no longer needs to override the >> _setLocale_ method and throw UnsupportedOperationException. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > cleanup existing typos Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15726#pullrequestreview-1630021636
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v5]
On Sat, 16 Sep 2023 07:37:37 GMT, Andrey Turbanov wrote: >> src/java.base/share/classes/sun/util/calendar/ImmutableGregorianDate.java >> line 160: >> >>> 158: unsupported(); >>> 159: } >>> 160: >> >> This removal does not look right. The class claims `immutable`, and yet it >> is now allowing setting the locale. > > There is no `setLocale` method in the base class anymore. So, it's not > possible to set locale in any classes which extend `BaseCalendar.Date`, > including `ImmutableGregorianDate` It is ok then. (missed the removal of the method in the abstract class). Now come to think of it, I would like them to be `sealed` to make sure they are not randomly subclassed, but that clean-up is for another day. - PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1328020279
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v5]
On Fri, 15 Sep 2023 23:31:16 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> cleanup existing typos > > src/java.base/share/classes/sun/util/calendar/ImmutableGregorianDate.java > line 160: > >> 158: unsupported(); >> 159: } >> 160: > > This removal does not look right. The class claims `immutable`, and yet it is > now allowing setting the locale. There is no `setLocale` method in the base class anymore. So, it's not possible to set locale in any classes which extend `BaseCalendar.Date`, including `ImmutableGregorianDate` - PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1327921070
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v5]
On Fri, 15 Sep 2023 20:24:35 GMT, Justin Lu wrote: >> Please review this PR which is a continuation of >> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused >> code from the _sun.util.Calendar_ classes. >> >> `forceStandardTime` is always false. >> >> In addition, `locale` is never by used by _CalendarDate_ or any inheritors >> and can be removed. >> >> As a result, _ImmutableGregorianDate_ no longer needs to override the >> _setLocale_ method and throw UnsupportedOperationException. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > cleanup existing typos I have to retract the PR approval for the said reason src/java.base/share/classes/sun/util/calendar/ImmutableGregorianDate.java line 160: > 158: unsupported(); > 159: } > 160: This removal does not look right. The class claims `immutable`, and yet it is now allowing setting the locale. - Changes requested by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15726#pullrequestreview-1629850335 PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1327862262
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v5]
> Please review this PR which is a continuation of > [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused > code from the _sun.util.Calendar_ classes. > > `forceStandardTime` is always false. > > In addition, `locale` is never by used by _CalendarDate_ or any inheritors > and can be removed. > > As a result, _ImmutableGregorianDate_ no longer needs to override the > _setLocale_ method and throw UnsupportedOperationException. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: cleanup existing typos - Changes: - all: https://git.openjdk.org/jdk/pull/15726/files - new: https://git.openjdk.org/jdk/pull/15726/files/2a2b0e7f..63ac1e9f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15726/head:pull/15726 PR: https://git.openjdk.org/jdk/pull/15726
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v4]
On Fri, 15 Sep 2023 18:54:19 GMT, Justin Lu wrote: >> Please review this PR which is a continuation of >> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused >> code from the _sun.util.Calendar_ classes. >> >> `forceStandardTime` is always false. >> >> In addition, `locale` is never by used by _CalendarDate_ or any inheritors >> and can be removed. >> >> As a result, _ImmutableGregorianDate_ no longer needs to override the >> _setLocale_ method and throw UnsupportedOperationException. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Clarify implementation after removal of if else block Marked as reviewed by naoto (Reviewer). src/java.base/share/classes/sun/util/calendar/AbstractCalendar.java line 171: > 169: // adjust time zone and daylight saving > 170: // 1) 2:30am during starting-DST transition is > 171: //intrepreted as 3:30am DT Not your change, but I think this is a typo of `interpreted` - PR Review: https://git.openjdk.org/jdk/pull/15726#pullrequestreview-1629657227 PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1327739161
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v4]
On Fri, 15 Sep 2023 19:05:12 GMT, Justin Lu wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clarify implementation after removal of if else block
>
> src/java.base/share/classes/sun/util/calendar/AbstractCalendar.java line 176:
>
>> 174: //as 1:30am DT/0:30am ST (before transition)
>> 175: if (zi instanceof ZoneInfo zInfo) {
>> 176: // Offset value adjusts accordingly depending on DST
>> status of date
>
> Historically, this `if else` has not been touched since the introduction of
> the class.
>
> The original code has a structure that one can presume follows the logic, if
> `isStandardTime()`, get a standard offset, otherwise get a day light saving
> offset. This is not the case.
>
> The code within the `else` statement is able to retrieve the correct offset
> if the date is in standard **or** in day light saving time (not just a day
> light saving offset, as the original code would imply). Consider the
> following example,
>
>
> // Where ms is calculated from the date: LA time zone at 3-13-2016 at 4 AM
> (daylight saving)
> zoneOffset = zInfo.getOffsetsByWall(ms, new int[2]);
> // returns the adjusted offset, -2520 (7 hours)
>
> // Where ms is calculated from the date: LA time zone at 1-13-2016 at 4 AM
> (standard)
> zoneOffset = zInfo.getOffsetsByWall(ms, new int[2]);
> // returns the standard offset, -2880 (8 hours)
>
> Removing this code is not only safe because `isStandardTime()` is always
> `false`
> - tiers 1-3 clean
> - breakpoint within the original `if` never breaks execution for JDK Calendar
> tests
>
> But we can also feel that the change is not suspicious since the code within
> the `else` block can produce a standard or daylight offset.
I suspect that the original design is to have this overridden by possible
subclasses if needed, but none has done so far (and I don't think it ever
will). So I think it is safe (and less complex) to remove these unused codes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1327738047
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v4]
On Fri, 15 Sep 2023 18:54:19 GMT, Justin Lu wrote:
>> Please review this PR which is a continuation of
>> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused
>> code from the _sun.util.Calendar_ classes.
>>
>> `forceStandardTime` is always false.
>>
>> In addition, `locale` is never by used by _CalendarDate_ or any inheritors
>> and can be removed.
>>
>> As a result, _ImmutableGregorianDate_ no longer needs to override the
>> _setLocale_ method and throw UnsupportedOperationException.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Clarify implementation after removal of if else block
src/java.base/share/classes/sun/util/calendar/AbstractCalendar.java line 176:
> 174: //as 1:30am DT/0:30am ST (before transition)
> 175: if (zi instanceof ZoneInfo zInfo) {
> 176: // Offset value adjusts accordingly depending on DST
> status of date
Historically, this `if else` has not been touched since the introduction of the
class.
The original code has a structure that one can presume follows the logic, if
`isStandardTime()`, get a standard offset, otherwise get a day light saving
offset. This is not the case.
The code within the `else` statement is able to retrieve the correct offset if
the date is in standard **or** in day light saving time (not just a day light
saving offset, as the original code would imply). Consider the following
example,
// Where ms is calculated from the date: LA time zone at 3-13-2016 at 4 AM
(daylight saving)
zoneOffset = zInfo.getOffsetsByWall(ms, new int[2]);
// returns the adjusted offset, -2520 (7 hours)
// Where ms is calculated from the date: LA time zone at 1-13-2016 at 4 AM
(standard)
zoneOffset = zInfo.getOffsetsByWall(ms, new int[2]);
// returns the standard offset, -2880 (8 hours)
Removing this code is not only safe because `isStandardTime()` is always `false`
- tiers 1-3 clean
- breakpoint within the original `if` never breaks execution for JDK Calendar
tests
But we can also feel that the change is not suspicious since the code within
the `else` block can produce a standard or daylight offset.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1327678366
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v4]
> Please review this PR which is a continuation of > [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused > code from the _sun.util.Calendar_ classes. > > `forceStandardTime` is always false. > > In addition, `locale` is never by used by _CalendarDate_ or any inheritors > and can be removed. > > As a result, _ImmutableGregorianDate_ no longer needs to override the > _setLocale_ method and throw UnsupportedOperationException. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Clarify implementation after removal of if else block - Changes: - all: https://git.openjdk.org/jdk/pull/15726/files - new: https://git.openjdk.org/jdk/pull/15726/files/5a3375ec..2a2b0e7f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=02-03 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15726/head:pull/15726 PR: https://git.openjdk.org/jdk/pull/15726
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v3]
On Wed, 13 Sep 2023 20:21:20 GMT, Justin Lu wrote: >> Please review this PR which is a continuation of >> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused >> code from the _sun.util.Calendar_ classes. >> >> `forceStandardTime` is always false. >> >> In addition, `locale` is never by used by _CalendarDate_ or any inheritors >> and can be removed. >> >> As a result, _ImmutableGregorianDate_ no longer needs to override the >> _setLocale_ method and throw UnsupportedOperationException. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Reflect review comments Marked as reviewed by aturbanov (Committer). - PR Review: https://git.openjdk.org/jdk/pull/15726#pullrequestreview-1625404590
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v3]
> Please review this PR which is a continuation of > [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused > code from the _sun.util.Calendar_ classes. > > `forceStandardTime` is always false. > > In addition, `locale` is never by used by _CalendarDate_ or any inheritors > and can be removed. > > As a result, _ImmutableGregorianDate_ no longer needs to override the > _setLocale_ method and throw UnsupportedOperationException. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Reflect review comments - Changes: - all: https://git.openjdk.org/jdk/pull/15726/files - new: https://git.openjdk.org/jdk/pull/15726/files/91bd5a3f..5a3375ec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=01-02 Stats: 5 lines in 1 file changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15726/head:pull/15726 PR: https://git.openjdk.org/jdk/pull/15726
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v2]
On Wed, 13 Sep 2023 19:46:08 GMT, Justin Lu wrote:
>> Please review this PR which is a continuation of
>> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused
>> code from the _sun.util.Calendar_ classes.
>>
>> `forceStandardTime` is always false.
>>
>> In addition, `locale` is never by used by _CalendarDate_ or any inheritors
>> and can be removed.
>>
>> As a result, _ImmutableGregorianDate_ no longer needs to override the
>> _setLocale_ method and throw UnsupportedOperationException.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove isStandardTime() and inline as false
src/java.base/share/classes/sun/util/calendar/AbstractCalendar.java line 169:
> 167: }
> 168: // adjust time zone and daylight saving
> 169: int[] offsets = new int[2];
Let's move array allocation only to `if (zi instanceof ZoneInfo) {` case
src/java.base/share/classes/sun/util/calendar/AbstractCalendar.java line 177:
> 175: //as 1:30am DT/0:30am ST (before transition)
> 176: if (zi instanceof ZoneInfo) {
> 177: zoneOffset = ((ZoneInfo)zi).getOffsetsByWall(ms,
> offsets);
let's use pattern matching for instanceof
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1325015164
PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1325015642
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v2]
On Wed, 13 Sep 2023 19:40:04 GMT, Justin Lu wrote:
>> Please review this PR which is a continuation of
>> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused
>> code from the _sun.util.Calendar_ classes.
>>
>> `forceStandardTime` is always false.
>>
>> In addition, `locale` is never by used by _CalendarDate_ or any inheritors
>> and can be removed.
>>
>> As a result, _ImmutableGregorianDate_ no longer needs to override the
>> _setLocale_ method and throw UnsupportedOperationException.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove isStandardTime() and inline as false
src/java.base/share/classes/sun/util/calendar/AbstractCalendar.java line 170:
> 168: // adjust time zone and daylight saving
> 169: int[] offsets = new int[2];
> 170: if (date.isStandardTime()) {
This seems a little suspicious considering `isStandardTime()` is always false.
However, at this point, there shouldn't be any behavioral changes probably.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1324987799
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set [v2]
> Please review this PR which is a continuation of > [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused > code from the _sun.util.Calendar_ classes. > > `forceStandardTime` is always false. > > In addition, `locale` is never by used by _CalendarDate_ or any inheritors > and can be removed. > > As a result, _ImmutableGregorianDate_ no longer needs to override the > _setLocale_ method and throw UnsupportedOperationException. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Remove isStandardTime() and inline as false - Changes: - all: https://git.openjdk.org/jdk/pull/15726/files - new: https://git.openjdk.org/jdk/pull/15726/files/d31bafae..91bd5a3f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=00-01 Stats: 34 lines in 3 files changed: 0 ins; 25 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/15726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15726/head:pull/15726 PR: https://git.openjdk.org/jdk/pull/15726
Re: RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set
On Wed, 13 Sep 2023 17:52:13 GMT, Justin Lu wrote:
> Please review this PR which is a continuation of
> [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused
> code from the _sun.util.Calendar_ classes.
>
> `forceStandardTime` is always false.
>
> In addition, `locale` is never by used by _CalendarDate_ or any inheritors
> and can be removed.
>
> As a result, _ImmutableGregorianDate_ no longer needs to override the
> _setLocale_ method and throw UnsupportedOperationException.
src/java.base/share/classes/sun/util/calendar/CalendarDate.java line 293:
> 291:
> 292:
> 293: public boolean isStandardTime() {
Can we remove(inline) this method?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15726#discussion_r1324902104
RFR: 8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set
Please review this PR which is a continuation of [JDK-6453901](https://bugs.openjdk.org/browse/JDK-6453901) to remove unused code from the _sun.util.Calendar_ classes. `forceStandardTime` is always false. In addition, `locale` is never by used by _CalendarDate_ or any inheritors and can be removed. As a result, _ImmutableGregorianDate_ no longer needs to override the _setLocale_ method and throw UnsupportedOperationException. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/15726/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15726&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8313813 Stats: 14 lines in 2 files changed: 0 ins; 13 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15726/head:pull/15726 PR: https://git.openjdk.org/jdk/pull/15726
