[GitHub] [thrift] ctubbsii commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r428664284



##########
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:
       I'm responding to the request on the mailing list for another pair of eyes on this PR.
   
   I agree with @Jens-G regarding the readability... readability is far more important than an every-so-slight performance benefit. Obfuscated code is very hard to maintain, even with good test cases.
   
   @joaopedroantonio 's suggestions are a great improvement, and preserves existing behavior, while adding the bounds checks when using offset and length.
   
   A unit test case that covers the edge cases would be a nice sanity check for this method.




----------------------------------------------------------------
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:
users@infra.apache.org