On Thu, 15 Feb 2024 19:44:42 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.
>
>
> This PR includes a lot of cleanup to the applyPattern implementation, 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.
src/java.base/share/classes/java/text/ChoiceFormat.java line 268:
> 266: for (int i = 0; i < newPattern.length(); ++i) {
> 267: char ch = newPattern.charAt(i);
> 268: switch(ch) {
Suggestion:
switch (ch) {
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1499095669
