On Thu, 4 Jun 2026 00:22:35 GMT, Naoto Sato <[email protected]> wrote:
>> Made changes to the invalid placeholder handling in >> `ListFormat.getInstance(String[])`, stemming from >> [JDK-8385736](https://bugs.openjdk.org/browse/JDK-8385736). >> >> This rejects duplicate placeholders and `"{2}"` outside the `three` pattern, >> with spec and test updates. Also includes minor Javadoc formatting cleanups. >> The invalid-long-pattern test added with >> [JDK-8385736](https://bugs.openjdk.org/browse/JDK-8385736) has been >> repurposed to cover the newly specified invalid-placeholder cases, because >> the repeated-placeholder input is now rejected immediately by >> duplicate-placeholder validation. >> >> Since this changes the behavior of the method, a corresponding CSR has been >> drafted. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > test mod Duplicate placeholder checking looks good to me. New test cases look good and exhaustive. Some other comments below. src/java.base/share/classes/java/text/ListFormat.java line 88: > 86: * Alternatively, Locale, Type, and/or Style independent instances > 87: * can be created with {@link #getInstance(String[])}. The String array > to the > 88: * method specifies the delimiting patterns for the {@code start}/{@code > middle}/{@code end} I think "The string array **passed** to the method" reads better. src/java.base/share/classes/java/text/ListFormat.java line 271: > 269: * three := > (three_before){0}three_between1{1}three_between2{2}(three_after) > 270: * } > 271: * If {@code two} or {@code three} pattern string is empty, it falls > back to Recommend: `If the {@code two} ...` src/java.base/share/classes/java/text/ListFormat.java line 687: > 685: var positions = new int[3]; > 686: for (int i = 0; i < positions.length; i++) { > 687: var ph = "{" + i + "}"; `ListFormat` uses `MessageFormat`, which means that any patterns taken by the former get passed to the latter. What would we expect to happen if `{3}` was passed as a pattern. Do we expect that to be printed out literally? Do we expect to reject it because it takes the form of a placeholder but exceeds the `0-2` value range? In the current form, depending on the number of elements, it could either be printed out literally, or it could be interpreted as a `MessageFormat` pattern. For example, String[] p = { "{3} {0}, {1}", "{0}, {1}", "{0}, and {1}", "", "" }; var lf = ListFormat.getInstance(p); lf.format(List.of("a", "b", "c", "d")); // -> d a, b, c, and d lf.format(List.of("a", "b", "c")); // -> {3} a, b, and c I would not be opposed to rejecting any placeholders that are not in the form of only 0, 1, and 2. I am going to guess that no locales rely on {HARD_CODED_NUMBER} as being a part of their list patterns. What do you think? test/jdk/java/text/Format/ListFormat/TestListFormat.java line 290: > 288: "{0} {1} {2}" > 289: }; > 290: patterns[index] = invalidPattern; I understand that the current implementation rejects this input immediately because it contains duplicate placeholders, but I think this `100_000` length case is still worth preserving as a regression test. I might retain it as a simple standalone test that preserves the long string input (and probably only needs to be checked against one pattern, not all). I say this because I don't think the test should be relying on an assumption of the current implementation. ------------- PR Review: https://git.openjdk.org/jdk/pull/31377#pullrequestreview-4430029952 PR Review Comment: https://git.openjdk.org/jdk/pull/31377#discussion_r3357985825 PR Review Comment: https://git.openjdk.org/jdk/pull/31377#discussion_r3357993644 PR Review Comment: https://git.openjdk.org/jdk/pull/31377#discussion_r3357959214 PR Review Comment: https://git.openjdk.org/jdk/pull/31377#discussion_r3357810289
