On Mon, 1 Nov 2021 22:27:08 GMT, Claes Redestad <redes...@openjdk.org> wrote:

>> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java 
>> line 3269:
>> 
>>> 3267:                 return false;
>>> 3268:             }
>>> 3269:             int val = value.intValue(); // NANO_OF_SECOND must fit in 
>>> an int and can't be negative
>> 
>> Unfortunately, this is not a valid assumption, and it affects the logic of 
>> the optimization more generally (methods where non-negative is assumed).
>> 
>> Basically, NANO_OF_SECOND (and all other fields in the formatter) can have 
>> any `long` value. Despite immediate appearances, the value is not limited to 
>> 0 to 999,999,999. This is because you are allowed to create an 
>> implementation of `Temporal` that returns values outside that range. No such 
>> class exists in the JDK, but such a class would be perfectly legal. As a 
>> related example, it would be perfectly value to write a time class that ran 
>> from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the 
>> formatter to be between 0 and 23.
>
> The commentary on this line could probably be improved, but this is in a 
> private printer-parser that will only be used for NANO_OF_SECOND and not any 
> arbitrary `TemporalField` (see line 704), thus I fail to see how this 
> assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to 
> 999,999,999).
> 
> I considered writing a more generic integral-fraction printer parser that 
> would optimize for any value-range that fits in an int, but seeing how 
> NANO_OF_SECOND is likely the only one used in practice and with a high demand 
> for better efficiency I opted to specialize for it more directly.

I see what you're saying that an arbitrary `Temporal` could define its own 
fields with its own ranges, but I would consider it a design bug if such an 
implementation at a whim redefines the value ranges of well-defined constants 
such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a 
`Temporal` would have to define its own enumeration of allowed `TemporalField`s.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6188

Reply via email to