On Fri, 15 Sep 2023 19:05:12 GMT, Justin Lu <j...@openjdk.org> 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, -25200000 (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, -28800000 (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
