On Tue, 5 Mar 2024 19:10:19 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains five additional commits
>> since the last revision:
>>
>> - cleanup/typo
>> - Merge branch 'master' into JDK-8326908-emptyPattern-OOME
>> - implement feedback + improve pattern related tests
>> - minor additions
>> - init
>
> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 60:
>
>> 58: dFmt.setMinimumFractionDigits(minFrac);
>> 59:
>> 60: String[] patterns = dFmt.toPattern().split("\\.");
>
> Instead of assuming/hardcoding the decimal separator,
> `DecimalFormatSymbols.getInstance(Locale.US).getDecimalSeparator()` may be
> better.
Actually, since `toPattern()` is non-localized, it will always use "." as
specified by DecimalFormat.
I removed the misleading comment `// Use a US dFmt, which uses '.' as the
decimal separator` that indicated we needed to use a DecimalFormatSymbols where
"." was the decimal separator. Can just simply use a DecimalFormat returned by
the default constructor.
> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 110:
>
>> 108: // 8326908: Verify that an empty pattern DecimalFormat does not
>> throw an
>> 109: // OutOfMemoryError when toPattern() is invoked. Behavioral change
>> of
>> 110: // MAXIMUM_INTEGER_DIGITS replaced with DOUBLE_FRACTION_DIGITS for
>> empty
>
> Should we explicitly check `new DecimalFormat("").getMaximumFractionDigits()
> == DOUBLE_FRACTION_DIGITS`?
Thanks for the review, added an explicit check
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513517348
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513517336
