On Mon, 11 Sep 2023 12:23:36 GMT, 温绍锦 <d...@openjdk.org> wrote:
>> Some codes in core libs are duplicated, including: >> java.util.DecimalDigits::DIGITS -> java.lang.StringLatin1.PACKED_DIGITS >> java.util.DecimalDigits::size -> java.lang.Long.stringSize >> >> We can reduce duplication through JavaLangAccess, which is also needed in >> other places, such as: >> https://github.com/openjdk/jdk/pull/15555 > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > move java.lang.StringLatin1::getChars to > jdk.internal.util.DecimalDigits::getCharsLatin1 In short: I suggest splitting the change of moving `getChars` `stringSize` into another patch; they are sufficiently distinct and can shrink this PR's size by a half at least. src/java.base/share/classes/java/lang/StringLatin1.java line 35: > 33: import java.util.stream.Stream; > 34: import java.util.stream.StreamSupport; > 35: import jdk.internal.util.ArraysSupport; Suggestion: import jdk.internal.util.ArraysSupport; import jdk.internal.util.DecimalDigits; Assuming you will restore the `getChars` here. src/java.base/share/classes/java/lang/StringLatin1.java line 42: > 40: import static java.lang.String.checkIndex; > 41: import static java.lang.String.checkOffset; > 42: import static jdk.internal.util.DecimalDigits.digitPair; Suggestion: Redundant. src/java.base/share/classes/java/lang/StringUTF16.java line 45: > 43: > 44: import static jdk.internal.util.DecimalDigits.digitPair; > 45: Suggestion: Per previous comments. Remember to add another line of import jdk.internal.util.DecimalDigits; above! src/java.base/share/classes/java/lang/StringUTF16.java line 1549: > 1547: i = q; > 1548: > 1549: int packed = (int) digitPair(r); Suggestion: int packed = (int) DecimalDigits.digitPair(r); src/java.base/share/classes/java/lang/StringUTF16.java line 1558: > 1556: // We know there are at most two digits left at this point. > 1557: if (i < -9) { > 1558: int packed = (int) digitPair(-i); Suggestion: int packed = (int) DecimalDigits.digitPair(-i); src/java.base/share/classes/java/lang/StringUTF16.java line 1596: > 1594: q = i / 100; > 1595: > 1596: int packed = (int) digitPair((int)((q * 100) - i)); Suggestion: int packed = (int) DecimalDigits.digitPair((int)((q * 100) - i)); src/java.base/share/classes/java/lang/StringUTF16.java line 1610: > 1608: q2 = i2 / 100; > 1609: > 1610: int packed = (int) digitPair((q2 * 100) - i2); Suggestion: int packed = (int) DecimalDigits.digitPair((q2 * 100) - i2); src/java.base/share/classes/java/lang/StringUTF16.java line 1622: > 1620: charPos -= 2; > 1621: > 1622: int packed = (int) digitPair(-i2); Suggestion: int packed = (int) DecimalDigits.digitPair(-i2); src/java.base/share/classes/jdk/internal/util/DecimalDigits.java line 43: > 41: } > 42: > 43: public int digits(long value, byte[] buffer, int index, MethodHandle > putCharMH) throws Throwable { `@Override` src/java.base/share/classes/jdk/internal/util/DecimalDigits.java line 143: > 141: * code after loop unrolling. > 142: */ > 143: public static int stringSize(int x) { I suggest splitting the moves of `stringSize` `getChars` into a new PR dependent on this one; your future date and time optimizations can depend on that one, which exposes stringSize. Having the DecimalDigits package move and `stringSize` `getChars` moves together complicates the file changes, and it's hard to detect if there's any accidental typo/malicious code in the new additions. src/java.base/share/classes/jdk/internal/util/OctalDigits.java line 68: > 66: > 67: @Override > 68: public int digits(long value, byte[] buffer, int index, MethodHandle > putCharMH) throws Throwable { Can you revert these IDE reformatting changes? So that we don't pollute the changeset and the Git blame log, making reviews and reverting patches easier. For example, only 3 lines of changes are truly necessary in this class: Changing the package, `public ` before `final class OctalDigits` and `static final Digits INSTANCE`. ------------- PR Review: https://git.openjdk.org/jdk/pull/15651#pullrequestreview-1619894567 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321648971 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321650127 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321646310 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321647751 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321647883 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321648031 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321648149 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321648317 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321487786 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321644436 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321640635
