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


##########
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
   // Set earlier before offset is changed
   final int end = offset + dataLen;
   
   // ...
   
   // Allow looping over (offset + 1 < end)
   final int loopEnd = end - 1;
   while (offset < loopEnd) {
               result = decodeOctet(data[offset++]) << BITS_PER_ENCODED_BYTE;
               result |= decodeOctet(data[offset++]);
   }
   
   // Consume final trailing byte
   if (offset < end) {
   
   ```
   
   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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to