[GitHub] [commons-codec] aherbert commented on a diff in pull request #135: Fix byte-skipping described in CODEC-305

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

aherbert commented on code in PR #135:
URL: https://github.com/apache/commons-codec/pull/135#discussion_r895118797


##########
src/main/java/org/apache/commons/codec/binary/Base16.java:
##########
@@ -177,26 +177,24 @@ void decode(final byte[] data, int offset, final int length, final Context conte
         if (dataLen < availableChars) {
             // we have 1/2 byte from previous invocation to decode
             result = (context.ibitWorkArea - 1) << BITS_PER_ENCODED_BYTE;
-            result |= decodeOctet(data[offset++]);

Review Comment:
   Can this avoid use of `offset + i++`:
   ```Java
   result |= decodeOctet(data[offset++]);
   i = 1;
   ```



##########
src/main/java/org/apache/commons/codec/binary/Base16.java:
##########
@@ -177,26 +177,24 @@ void decode(final byte[] data, int offset, final int length, final Context conte
         if (dataLen < availableChars) {
             // we have 1/2 byte from previous invocation to decode
             result = (context.ibitWorkArea - 1) << BITS_PER_ENCODED_BYTE;
-            result |= decodeOctet(data[offset++]);
-            i = 2;
+            result |= decodeOctet(data[offset + i++]);
 
             buffer[context.pos++] = (byte)result;
 
             // reset to empty-value for next invocation!
             context.ibitWorkArea = 0;
         }
 
-        while (i < charsToProcess) {
-            result = decodeOctet(data[offset++]) << BITS_PER_ENCODED_BYTE;
-            result |= decodeOctet(data[offset++]);
-            i += 2;
+        while (i + 1 < charsToProcess) {

Review Comment:
   I would prefer to not use `i + 1 < charsToProcess` on the loop condition.
   
   It seems the logic setting `charsToProcess` is incorrect when there is a previous half-byte. The value should not be final and can be decremented by 1 if `i` is set to 1:
   ```
   result |= decodeOctet(data[offset++]);
   i = 1;
   charsToProcess--;
   
   // ...
   
   while (i < charsToProcess) {
   ```
   
   This corrected logic still requires that offset is added to i: `data[offset + i++]`. Previously offset was incremented.
   
   This may be better served by computing the end point for offset:
   ```Java
   // End is an even number of chars added to the current offset
   final int end = offset + (dataLen & ~0x1);
   while (offset < end) {
               result = decodeOctet(data[offset++]) << BITS_PER_ENCODED_BYTE;
               result |= decodeOctet(data[offset++]);
   }
   
   if (offset < dataLen) {
   
   ```
   
   This logic would eliminate the requirement for `i` and `charsToProcess`. The logic to handle the ensureBufferSize can just use `availableChars`. Note: The code is untested.
   



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org