On Tue, 9 Apr 2024 01:34:56 GMT, Naoto Sato <na...@openjdk.org> 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