On Mon, 9 Jun 2025 22:46:50 GMT, Johannes Graham <d...@openjdk.org> wrote:
>> This PR replaces construction of intermediate strings to be parsed with more
>> direct manipulation of numbers. It also has a more streamlined mechanism of
>> handling `Long.MIN_VALUE` when parsing longs by using
>> `Long.parseUnsignedLong`
>>
>> As a small side-effect it also eliminates the use of a cached StringBuilder
>> in DigitList.
>>
>> Testing:
>> - GHA
>> - Local run of tier 2 and jtreg:jdk/java/text
>> - New benchmark: DecimalFormatParseBench
>
> Johannes Graham has updated the pull request incrementally with one
> additional commit since the last revision:
>
> catch ArithmeticException
Thanks for the improvements. I think we need to prioritize behavioral
compatibility with this change, so we will want to run the JCK tests as well
for the extra safety.
src/java.base/share/classes/jdk/internal/math/FloatingDecimal.java line 134:
> 132: * @return The double-precision value of the conversion
> 133: */
> 134: public static double parseDoubleDigits(int decExp, char[] digits,
> int length) throws NumberFormatException {
I don't think this method needs to declare `NFE` in the signature since it is
not reading in any strings so it should not throw.
src/java.base/share/classes/jdk/internal/math/FloatingDecimal.java line 1848:
> 1846: buf[i] = (byte) digits[i];
> 1847: }
> 1848: return new ASCIIToBinaryBuffer(false, decExp, buf, length);
Since `negSign` is always false, adding a _positive_ somewhere to the method
name might be helpful/explicit. Although you could argue that the array
parameter itself is telling enough, but one could misinterpret the char array
as including a sign character.
test/jdk/java/text/Format/DecimalFormat/CloneTest.java line 98:
> 96: }
> 97:
> 98: Object tempBuilder = valFromDigitList(original,
> "tempBuilder");
Since this is required as a direct result of this change, I think that warrants
adding the JBS bug ID to the Jtreg header.
test/micro/org/openjdk/bench/java/text/DecimalFormatParseBench.java line 28:
> 26: import java.text.DecimalFormat;
> 27: import java.text.ParseException;
> 28: import java.util.Locale;
Looks unused.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25644#pullrequestreview-2911579886
PR Review Comment: https://git.openjdk.org/jdk/pull/25644#discussion_r2136657118
PR Review Comment: https://git.openjdk.org/jdk/pull/25644#discussion_r2136676237
PR Review Comment: https://git.openjdk.org/jdk/pull/25644#discussion_r2136643849
PR Review Comment: https://git.openjdk.org/jdk/pull/25644#discussion_r2136644390
