On Tue, 15 Jul 2025 19:25:31 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Volkan Yazici has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Minimize the number of touched lines in `vmIntrinsics.hpp`
>> - Remove Markdown-styling in comments
>> - Improve wording of the `VerifyIntrinsicChecks` flag
>
> src/java.base/share/classes/java/lang/StringCoding.java line 36:
>
>> 34: import static java.util.Objects.requireNonNull;
>> 35: import static jdk.internal.util.Preconditions.AIOOBE_FORMATTER;
>> 36: import static jdk.internal.util.Preconditions.checkFromIndexSize;
>
> Avoid import static of methods, it makes the code harder to read and
> understand where the methods come from.
> Just import Preconditions.
Fixed in 2672f7c1ada.
> src/java.base/share/classes/java/lang/StringCoding.java line 162:
>
>> 160: if (c > '\u00FF')
>> 161: break;
>> 162: da[dp++] = (byte) c;
>
> Revert: Omit the space after (byte); there many more usages of "(byte)c" and
> this file does not need to drift from the prevailing style in
> StringUTF16.java AbstractStringBuilder, etc.
> Here and at line 195:
I've actually made these changes to match the code shared in intrinsics, e.g.,
`macroAssembler_x86.cpp`. Reverted them in 2672f7c1ada as you requested.
> src/java.base/share/classes/java/lang/StringCoding.java line 189:
>
>> 187:
>> 188: @IntrinsicCandidate
>> 189: private static int encodeAsciiArray0(char[] sa, int sp, byte[] da,
>> int dp, int len) {
>
> I rather prefer the readability of the src and dest parameters being on
> separate lines. (but not "{" by itself
> Suggestion:
>
> private static int encodeAsciiArray0(char[] sa, int sp,
> byte[] da, int dp, int len) {
Changed as requested in 2672f7c1ada.
> src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 43:
>
>> 41: import static java.util.Objects.requireNonNull;
>> 42: import static jdk.internal.util.Preconditions.AIOOBE_FORMATTER;
>> 43: import static jdk.internal.util.Preconditions.checkFromIndexSize;
>
> Avoid static imports of methods, it makes it harder to read and know where
> the methods are especially when crossing package boundaries.
Changed as requested in 2672f7c1ada.
> src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 177:
>
>> 175: if (c > '\u00FF')
>> 176: break;
>> 177: da[dp++] = (byte) c;
>
> Revert extra space.
Changed as requested in 2672f7c1ada.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208575103
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208578241
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208579144
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208579489
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208579689
