On Wed, 24 Sep 2025 21:34:28 GMT, Justin Lu <[email protected]> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflects review comments
>
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 2508:
>
>> 2506: @Override
>> 2507: public CompactNumberFormat clone() {
>> 2508: CompactNumberFormat other = (CompactNumberFormat)
>> super.clone();
>
> Maybe a comment indicating why we don't need to handle the `List<Patterns>`
> fields would be helpful for maintainers. (Since they are read-only after
> construction.) Otherwise we have to dig around the use sites to figure that
> out.
Good point. Added some explanation.
> test/jdk/java/text/Format/CompactNumberFormat/TestClone.java line 77:
>
>> 75: private static Stream<Arguments> referenceFields() throws
>> ClassNotFoundException {
>> 76: return Stream.of(
>> 77: Arguments.of("decimalFormat", DecimalFormat.class),
>
> We should also include `Arguments.of("symbols", DecimalFormatSymbols.class)`
> as an entry.
I was only testing mutable ones (as `symbols` is a constant), but as a
regression test for `clone()` impl, adding all cloned fields would be desired.
> test/jdk/java/text/Format/CompactNumberFormat/TestClone.java line 87:
>
>> 85: @MethodSource("referenceFields")
>> 86: void whiteBoxTest(String fieldName, Class<?> type) throws Throwable {
>> 87: var original= NumberFormat.getCompactNumberInstance(Locale.US,
>> NumberFormat.Style.SHORT);
>
> Suggestion:
>
> var original = NumberFormat.getCompactNumberInstance(Locale.US,
> NumberFormat.Style.SHORT);
For me this kind of typo is hard to detect, as IDE inserts its type in between
in the editor.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27475#discussion_r2377245099
PR Review Comment: https://git.openjdk.org/jdk/pull/27475#discussion_r2377247822
PR Review Comment: https://git.openjdk.org/jdk/pull/27475#discussion_r2377249888
