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