garydgregory commented on PR #1725:
URL: https://github.com/apache/commons-lang/pull/1725#issuecomment-4778783357
Very interesting discussion @alhudz, @sarankumarbaskar. Thank you for that.
I think we might have more to consider here. Going back to commit
2541a62def1c6e106ab4f4e0c72c45872dffca00 , we have this code path in
`CSU.regionMatches()`:
```java
if (cs instanceof String && substring instanceof String) {
return ((String) cs).regionMatches(ignoreCase, thisStart,
(String) substring, start, length);
}
```
I would argue that the question is whether that commit and the code until
now delegates to String as an optimization or a feature (it's both IMO). Java
has evolved since that commit as noted, and now presents different behavior as
of Java 9. (As a point of reference, our base requirement is Java 8 with the
goal of supporting current LTS releases, so 8, 11, 17, 21, 25.)
Passing in a `String`, a `StringBuilder`, or any CS implementation, to CSU
should have the same result (so the original test was correct I think). I claim
that if I call this CSU method, it is either because I have a variable typed as
a CS, or I have a concrete CS implementation and I want the CSU behavior to
future-proof a later change in my code to CS.
Obviously, calling the CSU method with a String or calling the String method
directly should have the same results.
All of this to say:
The non-String path in CSU should be a CS version of the `String`
implementation, such that tests verify that `String`, `StringBuilder`,
`StringBuffer`, `CharBuffer`, all have the same behavior. If that means that
the guts of the CSU method has a switch on `SystemUtils.IS_JAVA_1_8`, then so
bit it.
WDYT?
Tangent: Once this is resolved,
`org.apache.commons.codec.binary.CharSequenceUtils` should also be reviewed.
--
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]
