On Tue, 2 Jun 2026 17:11:41 GMT, Naoto Sato <[email protected]> wrote:
>> 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
The added comment makes the variable meaning very clear, so original looks good
to me.
>> 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.
I was going to say using `startsWith` combined with the 2-arg JUnit
`assertTrue` might work. Something along the lines of
`assertTrue(msg.startsWith(expected), () -> "Expected : " + expected + " but
got : " + msg);`
which is more clear in intent (IMO) and still has good failure output. But you
probably don't want to print the entire `100_000` length message, so you would
end up back with `Math.min` baked into the error message anyways. All that to
say, I'm fine with the current assertion.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31343#discussion_r3343313921
PR Review Comment: https://git.openjdk.org/jdk/pull/31343#discussion_r3343299792
