Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Wed, 10 Apr 2024 16:17:32 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no need for {@code} in javadoc
Thank you Naoto and Roger for the inputs and the reviews.
-
PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2060988355
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Wed, 10 Apr 2024 16:17:32 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no need for {@code} in javadoc
Marked as reviewed by rriggs (Reviewer).
The CSR is enough, no-one else except the filer will even notice.
-
PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-2004434418
PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2059771876
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Tue, 16 Apr 2024 06:08:00 GMT, Jaikiran Pai wrote: > Naoto, Roger, should we consider a release note for this change or is the CSR > itself enough? I think CSR is enough, as users' chance of encountering any issue is very slim. My $0.02 - PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2059524742
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Wed, 10 Apr 2024 16:17:32 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no need for {@code} in javadoc
The CSR for this has been approved. I'll integrate this PR tomorrow (around 24
hours from now).
Naoto, Roger, should we consider a release note for this change or is the CSR
itself enough?
-
PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2058299189
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Wed, 10 Apr 2024 16:17:32 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no need for {@code} in javadoc
I've now created the CSR https://bugs.openjdk.org/browse/JDK-8330135. I'll move
it to Finalized once I get a review for it.
-
PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2050930262
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Wed, 10 Apr 2024 16:17:32 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no need for {@code} in javadoc
Looks good, Jai. Thanks for the fix!
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-1992333798
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Wed, 10 Apr 2024 12:27:33 GMT, Naoto Sato wrote:
>> Hello Naoto, I had used `InstantSeconds` to keep it consistent with how a
>> similar doc is used for the `EPOCH_DAY` field. Let me know if you still
>> prefer this to be `INSTANT_SECONDS` and I will update it.
>
> With the @code tag, I initially thought it can be used programmatically, but
> apparently, it was simply an Enum name. Users may not notice it till they see
> the source (or run its `toString()`). I am fine with either way, but probably
> keep consistent with `EPOCH_DAY` where it is not using @code tag.
Hello Naoto, that's good point. I've updated the PR to remove `{@code}`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1559741398
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
> Can I please get a review of this change which proposes to fix the issue
> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>
> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
> for the epoch second.
>
> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value range
> to match the supported min and max values of `Instant` (as suggested by
> Stephen in that JBS issue). This commit also introduces a test to verify this
> change. This new test method as well as existing tests in tier1, tier2 and
> tier3 continue to pass with this change.
Jaikiran Pai has updated the pull request incrementally with one additional
commit since the last revision:
no need for {@code} in javadoc
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18674/files
- new: https://git.openjdk.org/jdk/pull/18674/files/6e535779..ca265686
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18674=02
- incr: https://webrevs.openjdk.org/?repo=jdk=18674=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18674.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18674/head:pull/18674
PR: https://git.openjdk.org/jdk/pull/18674
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:37:29 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8212895? >> >> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is >> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and >> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports >> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values >> for the epoch second. >> >> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value >> range to match the supported min and max values of `Instant` (as suggested >> by Stephen in that JBS issue). This commit also introduces a test to verify >> this change. This new test method as well as existing tests in tier1, tier2 >> and tier3 continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded > values Now that the changes have been finalized, I'll raise the CSR tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2047968321
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Wed, 10 Apr 2024 01:31:36 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/time/temporal/ChronoField.java line 590:
>>
>>> 588: * This is necessary to ensure interoperation between calendars.
>>> 589: *
>>> 590: * Range of {@code InstantSeconds} is between {@link Instant#MIN}
>>> and {@link Instant#MAX}
>>
>> Nit: `InstantSeconds` -> `INSTANT_SECONDS`
>
> Hello Naoto, I had used `InstantSeconds` to keep it consistent with how a
> similar doc is used for the `EPOCH_DAY` field. Let me know if you still
> prefer this to be `INSTANT_SECONDS` and I will update it.
With the @code tag, I initially thought it can be used programmatically, but
apparently, it was simply an Enum name. Users may not notice it till they see
the source (or run its `toString()`). I am fine with either way, but probably
keep consistent with `EPOCH_DAY` where it is not using @code tag.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1559346584
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 16:29:07 GMT, Naoto Sato wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded
>> values
>
> src/java.base/share/classes/java/time/temporal/ChronoField.java line 590:
>
>> 588: * This is necessary to ensure interoperation between calendars.
>> 589: *
>> 590: * Range of {@code InstantSeconds} is between {@link Instant#MIN}
>> and {@link Instant#MAX}
>
> Nit: `InstantSeconds` -> `INSTANT_SECONDS`
Hello Naoto, I had used `InstantSeconds` to keep it consistent with how a
similar doc is used for the `EPOCH_DAY` field. Let me know if you still prefer
this to be `INSTANT_SECONDS` and I will update it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1558644428
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:37:29 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded
> values
Thanks, Jai. The code change looks good. (Left a minor nit)
src/java.base/share/classes/java/time/temporal/ChronoField.java line 590:
> 588: * This is necessary to ensure interoperation between calendars.
> 589: *
> 590: * Range of {@code InstantSeconds} is between {@link Instant#MIN}
> and {@link Instant#MAX}
Nit: `InstantSeconds` -> `INSTANT_SECONDS`
-
PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-1989561065
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557961266
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:34:39 GMT, Jaikiran Pai wrote:
>> Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER,
>> ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
>> ` work?
>
> Hello Naoto, that's a very good point. I was too caught up with the constant
> values that it didn't occur to me that the `Instant.MIN` and `Instant.MAX`
> public fields could be used for this. I have followed your suggestion and
> updated the PR. I have also updated the javadoc to link to `Instant.MIN` and
> `Instant.MAX` as the supported epoch second range.
>
> The test continues to pass with this change and fails (as expected) without
> the source change.
Good, that was going to be my backup suggestion; trying to avoid a method call
even for the init. :)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557657347
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:37:29 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8212895? >> >> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is >> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and >> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports >> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values >> for the epoch second. >> >> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value >> range to match the supported min and max values of `Instant` (as suggested >> by Stephen in that JBS issue). This commit also introduces a test to verify >> this change. This new test method as well as existing tests in tier1, tier2 >> and tier3 continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded > values Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-1989084996
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 01:34:56 GMT, Naoto Sato wrote:
>> Hello Roger,
>>
>>> The code should reference the constants in Instant.java (though may need to
>>> make them package private.)
>>
>> The `ChronoField` class and the `Instant` class reside in separate packages,
>> so making these two fields in `Instant`, package private will not help. I
>> will have to make them public, which I think probably isn't a good idea.
>> Unless you think we should do it? There is one other place already in the
>> JDK where we have copy/pasted these values
>> `src/java.base/share/classes/java/nio/file/attribute/FileTime.java`, so
>> maybe we can continue with this copy/paste here too?
>>
>> As for the javadoc, after we decide about this field access detail, I'll
>> update it accordingly to note that the values min and max range complies
>> with the min and max epoch seconds supported by `Instant`.
>
> Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER,
> ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
> ` work?
Hello Naoto, that's a very good point. I was too caught up with the constant
values that it didn't occur to me that the `Instant.MIN` and `Instant.MAX`
public fields could be used for this. I have followed your suggestion and
updated the PR. I have also updated the javadoc to link to `Instant.MIN` and
`Instant.MAX` as the supported epoch second range.
The test continues to pass with this change and fails (as expected) without the
source change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557216492
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
> Can I please get a review of this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8212895? > > As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is > initialized to have a minimum and maximum values of `Long.MIN_VALUE` and > `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports > `-31557014167219200L` and `31556889864403199L` as minimum and maximum values > for the epoch second. > > The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value range > to match the supported min and max values of `Instant` (as suggested by > Stephen in that JBS issue). This commit also introduces a test to verify this > change. This new test method as well as existing tests in tier1, tier2 and > tier3 continue to pass with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded values - Changes: - all: https://git.openjdk.org/jdk/pull/18674/files - new: https://git.openjdk.org/jdk/pull/18674/files/1fea0f71..6e535779 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18674=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18674=00-01 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18674/head:pull/18674 PR: https://git.openjdk.org/jdk/pull/18674
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
On Tue, 9 Apr 2024 00:52:43 GMT, Jaikiran Pai wrote:
>> The code should reference the constants in Instant.java (though may need to
>> make them package private.)
>>
>> The javadoc can/should reference Instant.MIN and Instant.MAX (as the test
>> does).
>
> Hello Roger,
>
>> The code should reference the constants in Instant.java (though may need to
>> make them package private.)
>
> The `ChronoField` class and the `Instant` class reside in separate packages,
> so making these two fields in `Instant`, package private will not help. I
> will have to make them public, which I think probably isn't a good idea.
> Unless you think we should do it? There is one other place already in the JDK
> where we have copy/pasted these values
> `src/java.base/share/classes/java/nio/file/attribute/FileTime.java`, so maybe
> we can continue with this copy/paste here too?
>
> As for the javadoc, after we decide about this field access detail, I'll
> update it accordingly to note that the values min and max range complies with
> the min and max epoch seconds supported by `Instant`.
Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER,
ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
` work?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1556721063
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
On Mon, 8 Apr 2024 18:16:04 GMT, Roger Riggs wrote: >> src/java.base/share/classes/java/time/temporal/ChronoField.java line 590: >> >>> 588: * This is necessary to ensure interoperation between calendars. >>> 589: */ >>> 590: // ValueRange matches the min and max epoch second supported by >>> java.time.Instant >> >> Would we want to include this in the javadoc? > > The code should reference the constants in Instant.java (though may need to > make them package private.) > > The javadoc can/should reference Instant.MIN and Instant.MAX (as the test > does). Hello Roger, > The code should reference the constants in Instant.java (though may need to > make them package private.) The `ChronoField` class and the `Instant` class reside in separate packages, so making these two fields in `Instant`, package private will not help. I will have to make them public, which I think probably isn't a good idea. Unless you think we should do it? There is one other place already in the JDK where we have copy/pasted these values `src/java.base/share/classes/java/nio/file/attribute/FileTime.java`, so maybe we can continue with this copy/paste here too? As for the javadoc, after we decide about this field access detail, I'll update it accordingly to note that the values min and max range complies with the min and max epoch seconds supported by `Instant`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1556676053
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
On Mon, 8 Apr 2024 18:00:02 GMT, Naoto Sato wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8212895? >> >> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is >> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and >> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports >> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values >> for the epoch second. >> >> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value >> range to match the supported min and max values of `Instant` (as suggested >> by Stephen in that JBS issue). This commit also introduces a test to verify >> this change. This new test method as well as existing tests in tier1, tier2 >> and tier3 continue to pass with this change. > > src/java.base/share/classes/java/time/temporal/ChronoField.java line 590: > >> 588: * This is necessary to ensure interoperation between calendars. >> 589: */ >> 590: // ValueRange matches the min and max epoch second supported by >> java.time.Instant > > Would we want to include this in the javadoc? The code should reference the constants in Instant.java (though may need to make them package private.) The javadoc can/should reference Instant.MIN and Instant.MAX (as the test does). - PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1556243941
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
On Mon, 8 Apr 2024 09:52:39 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8212895? > > As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is > initialized to have a minimum and maximum values of `Long.MIN_VALUE` and > `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports > `-31557014167219200L` and `31556889864403199L` as minimum and maximum values > for the epoch second. > > The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value range > to match the supported min and max values of `Instant` (as suggested by > Stephen in that JBS issue). This commit also introduces a test to verify this > change. This new test method as well as existing tests in tier1, tier2 and > tier3 continue to pass with this change. src/java.base/share/classes/java/time/temporal/ChronoField.java line 590: > 588: * This is necessary to ensure interoperation between calendars. > 589: */ > 590: // ValueRange matches the min and max epoch second supported by > java.time.Instant Would we want to include this in the javadoc? - PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1556219119
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
On Mon, 8 Apr 2024 09:52:39 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8212895? > > As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is > initialized to have a minimum and maximum values of `Long.MIN_VALUE` and > `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports > `-31557014167219200L` and `31556889864403199L` as minimum and maximum values > for the epoch second. > > The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value range > to match the supported min and max values of `Instant` (as suggested by > Stephen in that JBS issue). This commit also introduces a test to verify this > change. This new test method as well as existing tests in tier1, tier2 and > tier3 continue to pass with this change. Given that this is a observable change through a public API, I think a csr will be needed for this. I'll draft one shortly. - PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2042488119
RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/JDK-8212895? As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is initialized to have a minimum and maximum values of `Long.MIN_VALUE` and `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports `-31557014167219200L` and `31556889864403199L` as minimum and maximum values for the epoch second. The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value range to match the supported min and max values of `Instant` (as suggested by Stephen in that JBS issue). This commit also introduces a test to verify this change. This new test method as well as existing tests in tier1, tier2 and tier3 continue to pass with this change. - Commit messages: - 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant Changes: https://git.openjdk.org/jdk/pull/18674/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18674=00 Issue: https://bugs.openjdk.org/browse/JDK-8212895 Stats: 15 lines in 2 files changed: 12 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18674/head:pull/18674 PR: https://git.openjdk.org/jdk/pull/18674
