On Fri, 13 Jun 2025 19:34:40 GMT, Johannes Graham <d...@openjdk.org> wrote:

>> Ah got it, I see your point. We would have goten underflow in 
>> `ASCIIToBinaryConverter.doubleValue()` for some extreme cases without a 
>> check. 
>> 
>> Is there a specific example you have that requires the switch to the newer 
>> check? Adding a comment along those lines might be helpful. Actually, I 
>> thought DigitList caps `decimalAt` to Integer.MIN/MAX, so then the first 
>> check you had would have been fine. (Maybe I am missing something?)
>
> I don't have a specific example, so I've reverted to my original check. I'm a 
> bit unsettled by the check for an extreme value later in `doubleValue()` 
> comparing against `MIN_DECIMAL_EXPONENT - 1`

IMO, the original check you had is easier to understand what is happening 
without further context, so I prefer your switch back. 

I think we are fine from (negative) "extreme values" in `doubleValue()` because 
of the check you have implemented in the first place. i.e. we avoid any 
potential underflow from `int exp = decExponent - kDigits;`. I think we do need 
a comment to accompany the check. (Why do we check? why not check the max 
exponent value?)

Also, should the check be against `MIN_DECIMAL_EXPONENT - 1` for consistency 
with `doubleValue()`? (Functionally, I don't think it matters.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25644#discussion_r2150586388

Reply via email to