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