Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8177819: DateTimeFormatterBuilder zone parsing should recognise DST #6527

Closed
wants to merge 5 commits into from

Conversation

naotoj
Copy link
Member

@naotoj naotoj commented Nov 23, 2021

This fix intends to honor the type (std/dst/generic) of parsed zone name for selecting the offset at the overlap. Corresponding CSR has also been drafted.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8177819: DateTimeFormatterBuilder zone parsing should recognise DST
  • JDK-8277049: ZonedDateTime parse in Fall DST transition fails to retain the correct zonename.
  • JDK-8277607: DateTimeFormatterBuilder zone parsing should recognise DST (CSR) ⚠️ Issue is not open.

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6527/head:pull/6527
$ git checkout pull/6527

Update a local copy of the PR:
$ git checkout pull/6527
$ git pull https://git.openjdk.java.net/jdk pull/6527/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6527

View PR using the GUI difftool:
$ git pr show -t 6527

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6527.diff

8277049: ZonedDateTime parse in Fall DST transition fails to retain the correct zonename.
@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 23, 2021

/csr

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 23, 2021

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Nov 23, 2021
@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 23, 2021

/issue add JDK-8277049

@openjdk openjdk bot added the csr label Nov 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

@naotoj this pull request will not be integrated until the CSR request JDK-8277607 for issue JDK-8177819 has been approved.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

@naotoj
Adding additional issue to issue list: 8277049: ZonedDateTime parse in Fall DST transition fails to retain the correct zonename..

@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

@naotoj The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

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.

@openjdk openjdk bot added core-libs i18n labels Nov 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 23, 2021

Copy link
Member

@JoeWang-Java JoeWang-Java left a comment

For the compatibility assessment, it looks like an incompatible change since apps that expect DST will get a different offset at the overlap. The risk is low, perhaps because of the rare use case? That additional explanation might be helpful.

* of {@link ChronoLocalDateTime#atZone(ZoneId)}. If the {@code ZoneId} was
* parsed from the zone name that does not imply daylight saving time, then
* {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} is issued
* to use the standard offset at the overlap, before forming the instant.
Copy link
Member

@JoeWang-Java JoeWang-Java Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the standard offset the subject instead of the withLaterOffsetAtOverlap method? Calling the method seems to be an impl detail to me. We might rephrase the sentence to sth. like:
If the {@code ZoneId} parsed does not indicate daylight saving time, the standard offset will be used at the local time-line overlap as specified in the {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} method to form the instant.

Copy link
Member Author

@naotoj naotoj Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the method, and made standard offset the subject of the sentence.

/**
* The parsed zone name type.
*/
int zoneNameType = -1;
Copy link
Member

@JoeWang-Java JoeWang-Java Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be an Enum if that helps with readability.

Copy link
Member Author

@naotoj naotoj Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the index needs some calculation, I left it as int. Changed to use the constant fields for better readability.

@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 24, 2021

Thanks, Joe. Refined the wording in the compatibility assessment in the CSR.

Copy link
Member

@JoeWang-Java JoeWang-Java left a comment

Thanks Naoto. The change Look good to me.

@@ -4897,13 +4903,13 @@ protected boolean prefixOf(CharSequence text, int off, int end) {
*/
private static class LENIENT extends CI {

private LENIENT(String k, String v, PrefixTree child) {
super(k, v, child);
private LENIENT(String k, String v, int t, PrefixTree child) {
Copy link
Contributor

@jodastephen jodastephen Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is class LENIENT actually in use?

Copy link
Member Author

@naotoj naotoj Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

* of {@link ChronoLocalDateTime#atZone(ZoneId)}.
* of {@link ChronoLocalDateTime#atZone(ZoneId)}. If the {@code ZoneId} was
* parsed from the zone name that does not indicate daylight saving time, then
* the standard offset will be used at the local time-line overlap.
Copy link
Contributor

@jodastephen jodastephen Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, I would read it as being the generic zone name that gets altered, rather than a zone name that explicitly indicates "winter" time. maybe:

"If the {@code ZoneId} was parsed from a zone name that indicates whether daylight saving time in in operation or not, then that fact will be used to select the correct offset at the local time-line overlap."

Copy link
Member Author

@naotoj naotoj Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as suggested.

@openjdk openjdk bot removed the csr label Nov 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 29, 2021

@naotoj 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:

8177819: DateTimeFormatterBuilder zone parsing should recognise DST
8277049: ZonedDateTime parse in Fall DST transition fails to retain the correct zonename.

Reviewed-by: joehw, scolebourne

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 93 new commits pushed to the master branch:

  • abaa073: 8277946: NMT: Deprecate and remove VM.native_memory shutdown jcmd command option
  • 37ff7f3: 8277866: gc/epsilon/TestMemoryMXBeans.java failed with wrong initial heap size
  • 8d7958e: 8277981: String Deduplication table is never cleaned up due to bad dead_factor_for_cleanup
  • bc6dce1: 8277736: G1: Allow forced evacuation failure of first N regions in collection set
  • 0c29ee5: 8274319: Replace usages of Collections.sort with List.sort call in jdk.jfr
  • f505396: 8277459: Add jwebserver tool
  • 84aa0a1: 8278047: Few javax/imageio test regressed after JDK-8262297 fix
  • fde0b95: 8277861: Terminally deprecate Thread.stop
  • 70d5dff: 8275326: C2: assert(no_dead_loop) failed: dead loop detected
  • 349328c: 8277777: [Vector API] assert(r->is_XMMRegister()) failed: must be in x86_32.ad
  • ... and 83 more: https://git.openjdk.java.net/jdk/compare/7b67a49f830b71ef129a4a3a9b0d994d58587edf...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 29, 2021
@naotoj
Copy link
Member Author

@naotoj naotoj commented Dec 1, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

Going to push as commit a363b7b.
Since your change was applied there have been 95 commits pushed to the master branch:

  • 9b3e672: 8278014: [vectorapi] Remove test run script
  • 1e9ed54: 8193682: Infinite loop in ZipOutputStream.close()
  • abaa073: 8277946: NMT: Deprecate and remove VM.native_memory shutdown jcmd command option
  • 37ff7f3: 8277866: gc/epsilon/TestMemoryMXBeans.java failed with wrong initial heap size
  • 8d7958e: 8277981: String Deduplication table is never cleaned up due to bad dead_factor_for_cleanup
  • bc6dce1: 8277736: G1: Allow forced evacuation failure of first N regions in collection set
  • 0c29ee5: 8274319: Replace usages of Collections.sort with List.sort call in jdk.jfr
  • f505396: 8277459: Add jwebserver tool
  • 84aa0a1: 8278047: Few javax/imageio test regressed after JDK-8262297 fix
  • fde0b95: 8277861: Terminally deprecate Thread.stop
  • ... and 85 more: https://git.openjdk.java.net/jdk/compare/7b67a49f830b71ef129a4a3a9b0d994d58587edf...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

@naotoj Pushed as commit a363b7b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs i18n integrated
3 participants