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