On Thu, 22 Feb 2024 22:50:03 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Please review this PR which handles an edge case pattern bug with
>> ChoiceFormat.
>>
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar|1.0#"
>> d.format(1) // unexpectedly returns ""
>>
>>
>> Not only does this lead to faulty formatting results, but breaks the
>> ChoiceFormat class invariant of duplicate limits.
>>
>> It would have been preferable if either an exception was thrown when the
>> ChoiceFormat was initially created, or the ChoiceFormat formatting 1
>> returned a value of "bar".
>>
>> For comparison,
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar"
>> d.format(1) // returns "bar"
>>
>>
>> After this change, the first code snippet now returns "bar" when formatting
>> 1 and discards the "baz" as the second code snippet does.
>>
>>
>> The specific fix for this bug is addressed with the following, on line 305,
>>
>> if (seg != Segment.FORMAT) {
>> // Discard incorrect portion and finish building cFmt
>> break;
>> }
>>
>> This prevents a limit/format entry from being built when the current parsing
>> mode has not yet processed the limit and relation. The previous
>> implementation would build an additional limit/format of 1 with an empty
>> string. The `break` allows for the discarding of the incorrect portion and
>> continuing. Alternatively an exception could have been thrown. For
>> consistency with the second code snippet, the former was picked.
>>
>> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
>> semi-related specification fix.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert enum change
LGTM, with a minor suggestion.
src/java.base/share/classes/java/text/ChoiceFormat.java line 342:
> 340: } else {
> 341: num = Double.parseDouble(str);
> 342: }
Could use enhanced switch?
-------------
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17883#pullrequestreview-1897207137
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1500096680
