Copilot commented on code in PR #1813:
URL: https://github.com/apache/commons-lang/pull/1813#discussion_r4287773304
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -649,6 +649,9 @@ public static String center(String str, final int size,
final char padChar) {
*
* <p>
* If the size is less than the String length, the String is returned. A
{@code null} String returns {@code null}. A negative size is treated as zero.
+ * If the requested padding length on either side would split a Unicode
surrogate pair in {@code padStr}, the padding is shortened by one {@code char}
+ * (via delegation to {@link #leftPad(String, int, String)} and {@link
#rightPad(String, int, String)}) so the result never contains an unpaired
+ * surrogate. In that case, the returned String has a length of {@code
size - 1}.
Review Comment:
The Javadoc claim that `center(..., padStr)` will return a String of length
`size - 1` in the surrogate-splitting case appears inaccurate. Because
`center()` delegates to `leftPad` and then `rightPad`, shortening on the first
operation can be compensated by the second, so the final length can still be
`size` (e.g., `center(\"a\", 3, \"😀\")` yields length 3 per the added
examples/tests). Recommend rewording to state the result may be shorter than
`size` (at most `size - 1`) rather than always `size - 1`.
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -5275,13 +5284,20 @@ public static String leftPad(final String str, final
int size, String padStr) {
}
final int padLen = padStr.length();
final int strLen = str.length();
- final int pads = size - strLen;
+ int pads = size - strLen;
if (pads <= 0) {
return str; // returns original String when possible
}
if (padLen == 1 && pads <= PAD_LIMIT) {
return leftPad(str, size, padStr.charAt(0));
}
+ // keep the cut off the middle of a surrogate pair so the result is
never left holding a lone surrogate
+ if (splitsSurrogatePair(padStr, pads % padLen)) {
Review Comment:
Consider computing `pads % padLen` once into a local variable (e.g.,
`remainder`) and skipping the `splitsSurrogatePair` call when the remainder is
`0`. A remainder of `0` means there is no partial `padStr` slice, so there’s
nothing to split; avoiding the call improves readability and prevents relying
on `splitsSurrogatePair` handling index `0` correctly.
##########
src/test/java/org/apache/commons/lang3/StringUtilsTest.java:
##########
@@ -1486,6 +1509,41 @@ void testOverlaySurrogatePair() {
}
}
+ private static void assertNoLoneSurrogates(final String result) {
+ for (int i = 0; i < result.length(); i++) {
+ final char ch = result.charAt(i);
+ if (Character.isHighSurrogate(ch)) {
+ assertTrue(i + 1 < result.length() &&
Character.isLowSurrogate(result.charAt(i + 1)),
+ "lone high surrogate in: " + result);
+ i++; // skip the paired low surrogate
+ } else {
+ assertFalse(Character.isLowSurrogate(ch), "lone low surrogate
in: " + result);
+ }
+ }
+ }
+
+ static Stream<TriFunction<String, Integer, String, String>>
testPadSurrogatePair() {
+ return Stream.of(
+ StringUtils::center,
+ StringUtils::leftPad,
+ StringUtils::rightPad);
+ }
+
+ @ParameterizedTest
+ @MethodSource
+ void testPadSurrogatePair(final TriFunction<String, Integer, String,
String> padFunction) {
Review Comment:
The `@MethodSource` factory method and the parameterized test method share
the same name (`testPadSurrogatePair`) and rely on overloading + JUnit’s
default lookup behavior. This works, but it’s easy to misread/accidentally
break during refactors. Recommend renaming the source method (e.g.,
`padFunctions()` / `padSurrogatePairFunctions()`) and referencing it explicitly
in `@MethodSource(\"...\")`.
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -7089,13 +7109,20 @@ public static String rightPad(final String str, final
int size, String padStr) {
}
final int padLen = padStr.length();
final int strLen = str.length();
- final int pads = size - strLen;
+ int pads = size - strLen;
if (pads <= 0) {
return str; // returns original String when possible
}
if (padLen == 1 && pads <= PAD_LIMIT) {
return rightPad(str, size, padStr.charAt(0));
}
+ // keep the cut off the middle of a surrogate pair so the result is
never left holding a lone surrogate
+ if (splitsSurrogatePair(padStr, pads % padLen)) {
Review Comment:
Same as in `leftPad(...)`: compute the remainder once and avoid calling
`splitsSurrogatePair` when `remainder == 0`. This makes the intent clearer and
avoids an unnecessary check in the common case where `pads` is a multiple of
`padLen`.
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -661,6 +664,8 @@ public static String center(String str, final int size,
final char padChar) {
* StringUtils.center("a", 4, "yz") = "yayz"
* StringUtils.center("abc", 7, null) = " abc "
* StringUtils.center("abc", 7, "") = " abc "
+ * StringUtils.center("a", 2, "😀") = "a"
+ * StringUtils.center("a", 3, "😀") = "a😀"
Review Comment:
Using a literal emoji in Javadoc examples can introduce build/tooling issues
if a non-UTF-8 source encoding is ever used (or if downstream tooling
mishandles unicode in Javadoc). Consider using escaped form (e.g.,
`\"\\uD83D\\uDE00\"`) in these examples for maximum portability/consistency
with the tests already using escapes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
