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 [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
