RFR: 8368328: CompactNumberFormat.clone does not produce independent instances [v2]

Naoto Sato naoto at openjdk.org
Wed Sep 24 22:55:41 UTC 2025


On Wed, 24 Sep 2025 21:34:28 GMT, Justin Lu <jlu at openjdk.org> 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


More information about the core-libs-dev mailing list