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:
[email protected]
