8313813: Field sun.util.calendar.CalendarDate#forceStandardTime is never set #15726
Conversation
|
👋 Welcome back jlu! A progress list of the required criteria for merging this PR into |
|
@justin-curtis-lu The following labels will be automatically applied to this pull request:
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. |
Webrevs
|
| @@ -295,7 +291,7 @@ public boolean isNormalized() { | |||
|
|
|||
|
|
|||
| public boolean isStandardTime() { | |||
There was a problem hiding this comment.
Can we remove(inline) this method?
| @@ -167,29 +167,16 @@ public long getTime(CalendarDate date) { | |||
| } | |||
| // adjust time zone and daylight saving | |||
| int[] offsets = new int[2]; | |||
| if (date.isStandardTime()) { | |||
There was a problem hiding this comment.
This seems a little suspicious considering isStandardTime() is always false.
However, at this point, there shouldn't be any behavioral changes probably.
| @@ -167,29 +167,16 @@ public long getTime(CalendarDate date) { | |||
| } | |||
| // adjust time zone and daylight saving | |||
| int[] offsets = new int[2]; | |||
There was a problem hiding this comment.
Let's move array allocation only to if (zi instanceof ZoneInfo) { case
| // 3) 1:30am during ending-DST transition is interpreted | ||
| // as 1:30am DT/0:30am ST (before transition) | ||
| if (zi instanceof ZoneInfo) { | ||
| zoneOffset = ((ZoneInfo)zi).getOffsetsByWall(ms, offsets); |
There was a problem hiding this comment.
let's use pattern matching for instanceof
| // 3) 1:30am during ending-DST transition is interpreted | ||
| // as 1:30am DT/0:30am ST (before transition) | ||
| if (zi instanceof ZoneInfo zInfo) { | ||
| // Offset value adjusts accordingly depending on DST status of date |
There was a problem hiding this comment.
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
ifnever 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.
There was a problem hiding this comment.
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.
| // 3) 1:30am during ending-DST transition is interpreted | ||
| // as 1:30am DT/0:30am ST (before transition) | ||
| if (zi instanceof ZoneInfo zInfo) { | ||
| // Offset value adjusts accordingly depending on DST status of date |
There was a problem hiding this comment.
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.
| zoneOffset = zi.getOffset(ms - zi.getRawOffset()); | ||
| } | ||
| // 1) 2:30am during starting-DST transition is | ||
| // intrepreted as 3:30am DT |
There was a problem hiding this comment.
Not your change, but I think this is a typo of interpreted
|
@justin-curtis-lu 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: 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 88 new commits pushed to the
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 |
| protected void setLocale(Locale loc) { | ||
| unsupported(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This removal does not look right. The class claims immutable, and yet it is now allowing setting the locale.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| protected void setLocale(Locale loc) { | ||
| unsupported(); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
|
/integrate |
|
Going to push as commit 373e37b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@justin-curtis-lu Pushed as commit 373e37b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this PR which is a continuation of JDK-6453901 to remove unused code from the sun.util.Calendar classes.
forceStandardTimeis always false.In addition,
localeis 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.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15726/head:pull/15726$ git checkout pull/15726Update a local copy of the PR:
$ git checkout pull/15726$ git pull https://git.openjdk.org/jdk.git pull/15726/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15726View PR using the GUI difftool:
$ git pr show -t 15726Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15726.diff
Webrev
Link to Webrev Comment