On Tue, 16 Apr 2024 21:14:28 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> 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
>
> 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`.
`CompactNumberFormat` does define its own instance methods such as
`setParseBigDecimal`, `setGroupingSize`, etc, so I think that this wording
should still be included.
> 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?
Thanks for the clarification, updated the wording.
> 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
As this is a localized class, and the class description mentions `Locale` in
various places, I feel like `Locale` should still be included as a `see` tag,
(even though I removed the locale link). I am fine either way, what do you
think?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004434
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004370
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004906
