On Tue, 9 Apr 2024 01:34:56 GMT, Naoto Sato <[email protected]> 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
