Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Disallow `UTC` -> `GMT` > - Merge branch 'master' into JDK-8285844 > - Minor fixup > - Allowed round-trips for offset-style ZoneIds. > - Fixed offsets in milliseconds, added test variations, refined Custom ID > definitions > - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all > ZoneOffsets and returns GMT unexpected Marked as reviewed by uschindler (Author). - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Disallow `UTC` -> `GMT` > - Merge branch 'master' into JDK-8285844 > - Minor fixup > - Allowed round-trips for offset-style ZoneIds. > - Fixed offsets in milliseconds, added test variations, refined Custom ID > definitions > - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all > ZoneOffsets and returns GMT unexpected Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Disallow `UTC` -> `GMT` > - Merge branch 'master' into JDK-8285844 > - Minor fixup > - Allowed round-trips for offset-style ZoneIds. > - Fixed offsets in milliseconds, added test variations, refined Custom ID > definitions > - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all > ZoneOffsets and returns GMT unexpected Marked as reviewed by scolebourne (Author). - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Disallow `UTC` -> `GMT` - Merge branch 'master' into JDK-8285844 - Minor fixup - Allowed round-trips for offset-style ZoneIds. - Fixed offsets in milliseconds, added test variations, refined Custom ID definitions - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/9722decd..11150ac5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8606=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8606=03-04 Stats: 8626 lines in 248 files changed: 4866 ins; 2323 del; 1437 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v4]
On Wed, 11 May 2022 18:30:32 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Minor fixup
>
> src/java.base/share/classes/java/util/TimeZone.java line 80:
>
>> 78: * {@code GMT} Sign Hours {@code :} Minutes
>> {@code :} Seconds
>> 79: * {@code GMT} Sign Hours {@code :} Minutes
>> 80: * {@code GMT} Sign Hours Minutes
>
> For hours and minutes, the format can be hh:mm or hhmm. Is it worth it to
> support hhmmss as well?
It would be consistent, but I don't see much requirement for it. I'll consider
if someone needs it.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v4]
On Wed, 11 May 2022 17:04:41 GMT, Naoto Sato wrote:
>> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
>> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
>> Corresponding CSR is also being drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Minor fixup
src/java.base/share/classes/java/util/TimeZone.java line 80:
> 78: * {@code GMT} Sign Hours {@code :} Minutes
> {@code :} Seconds
> 79: * {@code GMT} Sign Hours {@code :} Minutes
> 80: * {@code GMT} Sign Hours Minutes
For hours and minutes, the format can be hh:mm or hhmm. Is it worth it to
support hhmmss as well?
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v4]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Minor fixup - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/fcdaf512..9722decd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8606=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8606=02-03 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v3]
On Wed, 11 May 2022 09:00:45 GMT, Uwe Schindler wrote: >> I tried it out: If you create a ZoneId with prefixes "UT" and "UTC", they >> fail to convert to TimeZone. Same happens if you use this as String name in >> `TimeZone#getTimeZone(String)`. This is another bug / missing feature! It >> does not work with or without this PR. > > In short, we can only test what works. The test was mainly added to allow > roundtrips of `ZoneOffset` instances. The code intended to allow only offset-style ids that start with `GMT` (which is compatible with TimeZone's CustomID). Now I made changes to allow all offset-style zone ids. - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v3]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Allowed round-trips for offset-style ZoneIds. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/81a806e5..fcdaf512 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8606=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8606=01-02 Stats: 133 lines in 3 files changed: 70 ins; 61 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Wed, 11 May 2022 08:32:48 GMT, Uwe Schindler wrote: >> Can the test cover `UT` prefix as well? (This is another valid prefix in >> `ZoneId`) >> >> If this PR isn't meant to work with UTC prefix, can a test be added that >> proves it does *not* work. >> >> ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00", >> "GMT+01:00", "UT+01:00" - and all should have some form of associated test. > > I tried it out: If you create a ZoneId with prefixes "UT" and "UTC", they > fail to convert to TimeZone. Same happens if you use this as String name in > `TimeZone#getTimeZone(String)`. This is another bug / missing feature! It > does not work with or without this PR. In short, we can only test what works. The test was mainly added to allow roundtrips of `ZoneOffset` instances. - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Wed, 11 May 2022 08:10:56 GMT, Stephen Colebourne wrote: >> Added them except "UTC+...", as it is not recognizable as a Custom ID. > > Can the test cover `UT` prefix as well? (This is another valid prefix in > `ZoneId`) > > If this PR isn't meant to work with UTC prefix, can a test be added that > proves it does *not* work. > > ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00", > "GMT+01:00", "UT+01:00" - and all should have some form of associated test. I tried it out: If you create a ZoneId with prefixes "UT" and "UTC", they fail to convert to TimeZone. Same happens if you use this as String name in `TimeZone#getTimeZone(String)`. This is another bug / missing feature! It does not work with or without this PR. - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 17:43:07 GMT, Naoto Sato wrote:
>> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
>>
>>> 41: private Object[][] testZoneOffsets() {
>>> 42: return new Object[][] {
>>> 43: {ZoneId.of("Z"), 0},
>>
>> I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a
>> `ZoneOffset` instance, but maybe add also the other string variants with
>> prefix (`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data
>> items. Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
>
> Added them except "UTC+...", as it is not recognizable as a Custom ID.
Can the test cover `UT` prefix as well? (This is another valid prefix in
`ZoneId`)
If this PR isn't meant to work with UTC prefix, can a test be added that proves
it does *not* work.
ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00",
"GMT+01:00", "UT+01:00" - and all should have some form of associated test.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 17:43:24 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/util/TimeZone.java line 543:
>>
>>> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID +
>>> tzid, totalSecs);
>>> 542: } else {
>>> 543: return getTimeZone(tzid, true);
>>
>> Before the change in this PR, we used to prefix `GMT` to (non-custom
>> timezone ids) if the timezone id returned by `ZoneId#getId()` started with
>> the `+` or `-` sign, before calling `getTimeZone(modifiedTzid, true)`.
>> With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now
>> call `getTimeZone(originalTzid, true)`, without first checking/prefixing the
>> id with `GMT`. Is that an intentional change and would that potentially
>> cause `getTimeZone(String, boolean)` to return a different result?
>
> Yes, it is intentional. The `Time-zone IDs` section in the `ZoneId` class
> description is clear that zone id starting with "+/-" is a `ZoneOffset`
> instance. Other ZoneIds should have offsets with prefix or region-based ids.
I was stumbling on this, too, but it is only ZoneOffset isntances that have
those strange names without GMT/UTC prefix. Default ZoneId instances created
from text strings always have a prefix. If you call ZoneId.of() with an offset
only and no prefix it returns a ZoneOffset instance (see the test).
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Mon, 9 May 2022 22:29:50 GMT, Uwe Schindler wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed offsets in milliseconds, added test variations, refined Custom ID
>> definitions
>
> src/java.base/share/classes/java/util/TimeZone.java line 539:
>
>> 537: public static TimeZone getTimeZone(ZoneId zoneId) {
>> 538: String tzid = zoneId.getId(); // throws an NPE if null
>> 539: if (zoneId instanceof ZoneOffset zo) {
>
> I like this because it is much faster than the conversion to ZoneId and
> parsing it back! It is similar to use of SimpleTimeZone, but this is better
> as the returned timezone is unmodifiable, correct?
Yes, and it aligns with the other call site (line 588).
> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
>
>> 41: private Object[][] testZoneOffsets() {
>> 42: return new Object[][] {
>> 43: {ZoneId.of("Z"), 0},
>
> I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a
> `ZoneOffset` instance, but maybe add also the other string variants with
> prefix (`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data
> items. Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
Added them except "UTC+...", as it is not recognizable as a Custom ID.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 13:12:10 GMT, Jaikiran Pai wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed offsets in milliseconds, added test variations, refined Custom ID
>> definitions
>
> src/java.base/share/classes/java/util/TimeZone.java line 109:
>
>> 107: *
>> 108: * NormalizedCustomID:
>> 109: * {@code GMT} Sign TwoDigitHours {@code :}
>> Minutes [Seconds]
>
> Hello Naoto,
>
> Should this instead be: `... Minutes [{@code :} Seconds]` -
> i.e. should it have the `:` literal if seconds are present in the custom
> timezone id?
The colon is included in `Seconds` part below. I changed the part name to
`ColonSeconds` to make it clearer.
> src/java.base/share/classes/java/util/TimeZone.java line 543:
>
>> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID + tzid,
>> totalSecs);
>> 542: } else {
>> 543: return getTimeZone(tzid, true);
>
> Before the change in this PR, we used to prefix `GMT` to (non-custom timezone
> ids) if the timezone id returned by `ZoneId#getId()` started with the `+` or
> `-` sign, before calling `getTimeZone(modifiedTzid, true)`.
> With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now
> call `getTimeZone(originalTzid, true)`, without first checking/prefixing the
> id with `GMT`. Is that an intentional change and would that potentially cause
> `getTimeZone(String, boolean)` to return a different result?
Yes, it is intentional. The `Time-zone IDs` section in the `ZoneId` class
description is clear that zone id starting with "+/-" is a `ZoneOffset`
instance. Other ZoneIds should have offsets with prefix or region-based ids.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed offsets in milliseconds, added test variations, refined Custom ID definitions - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/843e86be..81a806e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8606=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8606=00-01 Stats: 13 lines in 2 files changed: 3 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected
On Mon, 9 May 2022 18:59:43 GMT, Naoto Sato wrote:
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
> Corresponding CSR is also being drafted.
src/java.base/share/classes/java/util/TimeZone.java line 543:
> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID + tzid,
> totalSecs);
> 542: } else {
> 543: return getTimeZone(tzid, true);
Before the change in this PR, we used to prefix `GMT` to (non-custom timezone
ids) if the timezone id returned by `ZoneId#getId()` started with the `+` or
`-` sign, before calling `getTimeZone(modifiedTzid, true)`.
With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now call
`getTimeZone(originalTzid, true)`, without first checking/prefixing the id with
`GMT`. Is that an intentional change and would that potentially cause
`getTimeZone(String, boolean)` to return a different result?
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected
On Mon, 9 May 2022 18:59:43 GMT, Naoto Sato wrote:
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
> Corresponding CSR is also being drafted.
src/java.base/share/classes/java/util/TimeZone.java line 109:
> 107: *
> 108: * NormalizedCustomID:
> 109: * {@code GMT} Sign TwoDigitHours {@code :}
> Minutes [Seconds]
Hello Naoto,
Should this instead be: `... Minutes [{@code :} Seconds]` - i.e.
should it have the `:` literal if seconds are present in the custom timezone id?
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected
On Mon, 9 May 2022 18:59:43 GMT, Naoto Sato wrote:
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
> Corresponding CSR is also being drafted.
Marked as reviewed by uschindler (Author).
src/java.base/share/classes/java/util/TimeZone.java line 539:
> 537: public static TimeZone getTimeZone(ZoneId zoneId) {
> 538: String tzid = zoneId.getId(); // throws an NPE if null
> 539: if (zoneId instanceof ZoneOffset zo) {
I like this because it is much faster than the conversion to ZoneId and parsing
it back! It is similar to use of SimpleTimeZone, but this is better as the
returned timezone is unmodifiable, correct?
test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
> 41: private Object[][] testZoneOffsets() {
> 42: return new Object[][] {
> 43: {ZoneId.of("Z"), 0},
I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a
`ZoneOffset` instance, but maybe add also the other string variants with prefix
(`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data items.
Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected
This is to extend the `Custom ID`s in `java.util.TimeZone` class to support second-level resolution, enabling round trips with `java.time.ZoneOffset`s. Corresponding CSR is also being drafted. - Commit messages: - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected Changes: https://git.openjdk.java.net/jdk/pull/8606/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8606=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285844 Stats: 145 lines in 4 files changed: 98 ins; 1 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
