On Tue, 16 Apr 2024 17:12:06 GMT, Justin Lu <j...@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
