RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]

[Naoto Sato]

On Tue, 16 Apr 2024 17:12:06 GMT, Justin Lu <jlu at openjdk.org> wrote:

>> Please review this PR which is a large spec reformatting for _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat and CompactNumberFormat.
>> 
>> The motivation for this change was the difficulty of readability for these classes. This PR consists of changes such as better separating the text into sections with headers, advising to skip the pattern related sections if not needed, and general wording improvements.
>> 
>> To help with reviewing I have attached a [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) as well as the built docs: [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), and [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html);
>> 
>> I will update the CSR once the wording is finalized.
>
> Justin Lu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - merge master and add setStrict() to nFmt class spec
>  - implement suggestions from dFmt review
>  - implement suggestions from cnFmt review
>  - implement suggestions from nFmt review
>  - init

Thanks for the changes. Looks much better.

src/java.base/share/classes/java/text/CompactNumberFormat.java line 78:

> 76:  * installed. Thus, to use an instance method defined by {@code CompactNumberFormat},
> 77:  * the {@code NumberFormat} returned by the factory method should first be type
> 78:  * checked before cast to {@code CompactNumberFormat}. If the installed locale-sensitive

Since `CompactNumberFormat` does not provide its own instance methods (i.e., instance methods are exactly the same as the parent NumberFormat), "instance methods defined by CompactNF" does not make much sense, which is different for `DecimalFormat`.

src/java.base/share/classes/java/text/CompactNumberFormat.java line 80:

> 78:  * checked before cast to {@code CompactNumberFormat}. If the installed locale-sensitive
> 79:  * service implementation does not support the given locale, {@link Locale#ROOT}
> 80:  * will be used as a fallback.

Same with `NumberFormat`.

src/java.base/share/classes/java/text/DecimalFormat.java line 88:

> 86:  * NumberFormat nFmt = NumberFormat.getCurrencyInstance(Locale.US);
> 87:  * if (nFmt instanceof DecimalFormat dFmt) {
> 88:  *     // cast to DecimalFormat to use setPositiveSuffix(String)

This comment seems incorrect. No casting is made here.

src/java.base/share/classes/java/text/DecimalFormat.java line 421:

> 419:  * @see          DecimalFormatSymbols
> 420:  * @see          ParsePosition
> 421:  * @see          Locale

Can be removed

src/java.base/share/classes/java/text/NumberFormat.java line 91:

> 89:  * for example, {@link #getIntegerInstance(Locale)}. If the installed locale-sensitive
> 90:  * service implementation does not support the given {@code Locale}, {@link Locale#ROOT}
> 91:  * will be used as the fallback {@code Locale}.

The fallback traverses the parent locales, and checks if it is supported or not. So it may or may not reach Locale.ROOT. How about "it looks up the parent locale chain and uses the one that is supported" or something along the lines?

src/java.base/share/classes/java/text/NumberFormat.java line 235:

> 233:  * @see          ChoiceFormat
> 234:  * @see          CompactNumberFormat
> 235:  * @see          Locale

Could be removed as the link is now gone

-------------

PR Review: https://git.openjdk.org/jdk/pull/18731#pullrequestreview-2004548849
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567948678
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567949084
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567963269
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567966048
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567910502
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567937751