Copilot commented on code in PR #1813:
URL: https://github.com/apache/commons-lang/pull/1813#discussion_r4251315519
##########
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 shown is on the `center(String, int, char)` overload, but it
refers to `padStr` and includes examples that use the `String`-padding overload
(`center("a", 2, "π")`). This is misleading because supplementary characters
canβt be passed as a single `char` (they require a surrogate pair / `String`).
Move this surrogate-pair discussion + examples to the `center(String, int,
String)` overload Javadoc, and keep the `char` overload documentation focused
on `padChar` (or explicitly note that supplementary characters canβt be
represented by one `char`).
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -5246,7 +5251,9 @@ public static String leftPad(final String str, final int
size, final char padCha
* Left pad a String with a specified String.
*
* <p>
- * Pad to a size of {@code size}.
+ * Pad to a size of {@code size}. If the requested padding length would
split a Unicode surrogate pair in {@code padStr},
+ * the padding is shortened by one {@code char} so the result never
contains an unpaired surrogate. In that case, the
+ * returned String has a length of {@code size - 1}.
Review Comment:
These Javadoc updates appear in the `leftPad(String, int, char)` /
`rightPad(String, int, char)` sections (based on the surrounding signatures),
but they refer to `padStr` and show examples using the `String`-padding
overloads. This should be relocated to the `leftPad(String, int, String)` and
`rightPad(String, int, String)` overload documentation; otherwise it
incorrectly suggests callers can pass an emoji as a `char`.
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -5259,6 +5266,8 @@ public static String leftPad(final String str, final int
size, final char padCha
* StringUtils.leftPad("bat", -1, "yz") = "bat"
* StringUtils.leftPad("bat", 5, null) = " bat"
* StringUtils.leftPad("bat", 5, "") = " bat"
+ * StringUtils.leftPad("a", 2, "π") = "a"
+ * StringUtils.leftPad("a", 3, "π") = "πa"
Review Comment:
These Javadoc updates appear in the `leftPad(String, int, char)` /
`rightPad(String, int, char)` sections (based on the surrounding signatures),
but they refer to `padStr` and show examples using the `String`-padding
overloads. This should be relocated to the `leftPad(String, int, String)` and
`rightPad(String, int, String)` overload documentation; otherwise it
incorrectly suggests callers can pass an emoji as a `char`.
##########
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:
The Javadoc shown is on the `center(String, int, char)` overload, but it
refers to `padStr` and includes examples that use the `String`-padding overload
(`center("a", 2, "π")`). This is misleading because supplementary characters
canβt be passed as a single `char` (they require a surrogate pair / `String`).
Move this surrogate-pair discussion + examples to the `center(String, int,
String)` overload Javadoc, and keep the `char` overload documentation focused
on `padChar` (or explicitly note that supplementary characters canβt be
represented by one `char`).
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -7060,7 +7076,9 @@ public static String rightPad(final String str, final int
size, final char padCh
* Right pad a String with a specified String.
*
* <p>
- * The String is padded to the size of {@code size}.
+ * The String is padded to the size of {@code size}. If the requested
padding length would split a Unicode surrogate pair
+ * in {@code padStr}, the padding is shortened by one {@code char} so the
result never contains an unpaired surrogate.
+ * In that case, the returned String has a length of {@code size - 1}.
Review Comment:
These Javadoc updates appear in the `leftPad(String, int, char)` /
`rightPad(String, int, char)` sections (based on the surrounding signatures),
but they refer to `padStr` and show examples using the `String`-padding
overloads. This should be relocated to the `leftPad(String, int, String)` and
`rightPad(String, int, String)` overload documentation; otherwise it
incorrectly suggests callers can pass an emoji as a `char`.
##########
src/main/java/org/apache/commons/lang3/StringUtils.java:
##########
@@ -7073,6 +7091,8 @@ public static String rightPad(final String str, final int
size, final char padCh
* StringUtils.rightPad("bat", -1, "yz") = "bat"
* StringUtils.rightPad("bat", 5, null) = "bat "
* StringUtils.rightPad("bat", 5, "") = "bat "
+ * StringUtils.rightPad("a", 2, "π") = "a"
+ * StringUtils.rightPad("a", 3, "π") = "aπ"
Review Comment:
These Javadoc updates appear in the `leftPad(String, int, char)` /
`rightPad(String, int, char)` sections (based on the surrounding signatures),
but they refer to `padStr` and show examples using the `String`-padding
overloads. This should be relocated to the `leftPad(String, int, String)` and
`rightPad(String, int, String)` overload documentation; otherwise it
incorrectly suggests callers can pass an emoji as a `char`.
##########
src/test/java/org/apache/commons/lang3/StringUtilsTest.java:
##########
@@ -340,6 +340,37 @@ void testCenter_StringIntString() {
assertEquals(" abc ", StringUtils.center("abc", 7, ""));
}
+ @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);
+ 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);
+ }
+ }
Review Comment:
The βno lone surrogateβ validation loop is duplicated across
`testCenterSurrogatePair`, `testLeftPadSurrogatePair`, and
`testRightPadSurrogatePair`. Consider extracting it into a private helper
(e.g., `assertNoLoneSurrogates(String result)`) to reduce duplication and make
future changes (like improving failure diagnostics) apply consistently across
all three tests.
--
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]
