joaopedroantonio commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r429193227
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset +
length))) < 0) {
Review comment:
Maybe I'm being picky here but I think we should keep the
IllegalArgumentException when `length < 0`, for two reasons:
1. The semantics of IndexOutOfBoundsException and the
IllegalArgumentException are different and I think `length < 0` is a case of
the latter.
2. Neither of those exceptions is checked and changing the type of exception
when `length < 0` will not be explicit in compile time. I'd consider this a
breaking change in the API.
WYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
