On Tue, 2 Jun 2026 06:22:33 GMT, Justin Lu <[email protected]> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressing review comments
>
> src/java.base/share/classes/java/text/ListFormat.java line 114:
>
>> 112: private static final int THREE = 4;
>> 113: private static final int PATTERN_ARRAY_LENGTH = THREE + 1;
>> 114: private static final int PLACEHOLDER_LENGTH = 3;
>
> Nit: Would not be opposed to `PLACEHOLDER_TOKEN_LENGTH` here. To me, it makes
> it clearer that this is the length of the placeholder token text, not an
> arbitrary placeholder value.
Since `TOKEN` is not used elsewhere (and the LDML spec itself) I would rather
keep the original
> src/java.base/share/classes/java/text/ListFormat.java line 668:
>
>> 666: * @param pattern pattern string to parse
>> 667: */
>> 668: private static int[] findPlaceholders(String pattern) {
>
> Extra placeholder token behavior looks a little spotty (and was before as
> well, I believe). For example, given a `START` pattern, a duplicate "{1}"
> placeholder gets rejected because of the `placeholderPositions[1] +
> PLACEHOLDER_LENGTH == pattern.length()` check in `init()`. However, a
> duplicate "{0}" goes right through, for example "{0}, {0} {1}". I don't see
> any test cases or specification that hints at what to expect for that. Is
> this worth standardizing or documenting in a separate issue?
Good point. Will address this in a separate issue:
[JDK-8385834](https://bugs.openjdk.org/browse/JDK-8385834)
> test/jdk/java/text/Format/ListFormat/TestListFormat.java line 282:
>
>> 280: default -> throw new RuntimeException("invalid index");
>> 281: });
>> 282: assertEquals(expected, msg.substring(0, Math.min(msg.length(),
>> expected.length())));
>
> The `Math.min` tripped me up here. It looks to me that `expected` is always
> going to be shorter than `msg`, since `msg` includes the faulty pattern and
> `expected` is just the base error message. Should we just check that `msg`
> starts with `expected` using `startsWith` instead?
Initially I used `startsWith` for comparison, but it turned out that the error
messages were not logged, which was inconvenient for debugging.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31343#discussion_r3343070566
PR Review Comment: https://git.openjdk.org/jdk/pull/31343#discussion_r3343029862
PR Review Comment: https://git.openjdk.org/jdk/pull/31343#discussion_r3343046326
