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