Copilot commented on code in PR #1813:
URL: https://github.com/apache/commons-lang/pull/1813#discussion_r4270760617
##########
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 issue as in `leftPad`: `pads % padLen` may be `0`. If
`splitsSurrogatePair` doesn’t explicitly handle index `0`, this can throw at
runtime. Add a `splitPos != 0` guard (or equivalent) before calling
`splitsSurrogatePair`.
##########
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:
For `center(String, int, String)`, the Javadoc claims the result length
becomes `size - 1` in the surrogate-pair case. Since centering pads both sides
via `leftPad` and `rightPad`, it’s possible (depending on the computed
left/right pad sizes and `padStr` length) for both sides to hit the
surrogate-split adjustment, producing `size - 2`. Recommend rewording to
something like “may be shorter than `size` (by 1 on either side where a split
would occur)” or explicitly document the possible range so callers don’t rely
on `size - 1` being the only shortened outcome.
##########
src/test/java/org/apache/commons/lang3/StringUtilsTest.java:
##########
@@ -340,6 +340,41 @@ void testCenter_StringIntString() {
assertEquals(" abc ", StringUtils.center("abc", 7, ""));
}
+ private 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);
+ }
+ }
+ }
+
+ @Test
+ void testCenterSurrogatePair() {
+ final String grin = "😀";
+ assertEquals("a", StringUtils.center("a", 2, grin));
+ assertEquals("a😀", StringUtils.center("a", 3, grin));
+ assertEquals("a😀", StringUtils.center("a", 4, grin));
+ assertEquals("😀a😀", StringUtils.center("a", 5, grin));
+ assertEquals("😀a😀", StringUtils.center("a", 6, grin));
+ assertEquals("😀a😀😀", StringUtils.center("a", 7, grin));
+
+ final String[] testStrings = {"", "a", "hello", grin, "a" + grin +
"b"};
+ final String[] padStrings = {grin, "a" + grin, grin + "b", "a" + grin
+ "b", grin + "🎉"};
Review Comment:
These tests embed non-BMP characters directly as source literals. If the
build/tooling ever runs with a non-UTF-8 source encoding (or misconfigured
compiler/source encoding), this can lead to compilation/runtime mismatches.
Using explicit surrogate escapes (e.g., `"\uD83D\uDE00"`) or `new
String(Character.toChars(0x1F600))` makes the tests encoding-robust across
environments.
##########
src/test/java/org/apache/commons/lang3/StringUtilsTest.java:
##########
@@ -340,6 +340,41 @@ void testCenter_StringIntString() {
assertEquals(" abc ", StringUtils.center("abc", 7, ""));
}
+ private 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);
+ }
+ }
+ }
+
+ @Test
+ void testCenterSurrogatePair() {
+ final String grin = "😀";
+ assertEquals("a", StringUtils.center("a", 2, grin));
+ assertEquals("a😀", StringUtils.center("a", 3, grin));
+ assertEquals("a😀", StringUtils.center("a", 4, grin));
+ assertEquals("😀a😀", StringUtils.center("a", 5, grin));
+ assertEquals("😀a😀", StringUtils.center("a", 6, grin));
+ assertEquals("😀a😀😀", StringUtils.center("a", 7, grin));
+
+ final String[] testStrings = {"", "a", "hello", grin, "a" + grin +
"b"};
+ final String[] padStrings = {grin, "a" + grin, grin + "b", "a" + grin
+ "b", grin + "🎉"};
Review Comment:
These tests embed non-BMP characters directly as source literals. If the
build/tooling ever runs with a non-UTF-8 source encoding (or misconfigured
compiler/source encoding), this can lead to compilation/runtime mismatches.
Using explicit surrogate escapes (e.g., `"\uD83D\uDE00"`) or `new
String(Character.toChars(0x1F600))` makes the tests encoding-robust across
environments.
##########
src/test/java/org/apache/commons/lang3/StringUtilsTest.java:
##########
@@ -340,6 +340,41 @@ void testCenter_StringIntString() {
assertEquals(" abc ", StringUtils.center("abc", 7, ""));
}
+ private 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);
+ }
+ }
+ }
+
+ @Test
+ void testCenterSurrogatePair() {
+ final String grin = "😀";
+ assertEquals("a", StringUtils.center("a", 2, grin));
+ assertEquals("a😀", StringUtils.center("a", 3, grin));
+ assertEquals("a😀", StringUtils.center("a", 4, grin));
+ assertEquals("😀a😀", StringUtils.center("a", 5, grin));
+ assertEquals("😀a😀", StringUtils.center("a", 6, grin));
+ assertEquals("😀a😀😀", StringUtils.center("a", 7, grin));
+
+ final String[] testStrings = {"", "a", "hello", grin, "a" + grin +
"b"};
+ final String[] padStrings = {grin, "a" + grin, grin + "b", "a" + grin
+ "b", grin + "🎉"};
+ for (final String str : testStrings) {
+ for (final String padStr : padStrings) {
+ for (int size = 0; size <= 15; size++) {
+ final String result = StringUtils.center(str, size,
padStr);
+ assertNoLoneSurrogates(result);
+ }
+ }
+ }
Review Comment:
Very similar nested-loop “no lone surrogates” fuzz logic is duplicated
across the new `center`, `leftPad`, and `rightPad` tests. Consider extracting a
small private helper (e.g., takes a `BiFunction<String,Integer,String>`/lambda
or an enum indicating which API to call) to reduce duplication and keep future
additions (more sizes/strings) consistent across all three cases.
##########
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:
`pads % padLen` can be `0` when the padding length is an exact multiple of
`padLen`. If `splitsSurrogatePair` assumes the index is within `1..padLen-1` (a
common implementation checks `pos-1` and `pos`), passing `0` can cause an
out-of-bounds access. Consider guarding the call (e.g., compute `final int
splitPos = pads % padLen;` and only call `splitsSurrogatePair` when `splitPos
!= 0`). This avoids a potential runtime exception while keeping the behavior
unchanged when no cut occurs.
--
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]
