Copilot commented on code in PR #10127:
URL: https://github.com/apache/ozone/pull/10127#discussion_r3200035708
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/ChecksumByteBuffer.java:
##########
@@ -47,81 +42,4 @@ public interface ChecksumByteBuffer extends Checksum {
default void update(byte[] b, int off, int len) {
update(ByteBuffer.wrap(b, off, len).asReadOnlyBuffer());
}
-
- /**
- * An abstract class implementing {@link ChecksumByteBuffer}
- * with a 32-bit checksum and a lookup table.
- */
- @SuppressWarnings("innerassignment")
- abstract class CrcIntTable implements ChecksumByteBuffer {
- /** Current CRC value with bit-flipped. */
- private int crc;
-
- CrcIntTable() {
- reset();
- Preconditions.assertTrue(getTable().length == 8 * (1 << 8));
- }
-
- abstract int[] getTable();
-
- @Override
- public final long getValue() {
- return (~crc) & 0xffffffffL;
- }
-
- @Override
- public final void reset() {
- crc = 0xffffffff;
- }
-
- @Override
- public final void update(int b) {
- crc = (crc >>> 8) ^ getTable()[(((crc ^ b) << 24) >>> 24)];
- }
-
- @Override
- public final void update(ByteBuffer b) {
- crc = update(crc, b, getTable());
- }
-
- private static int update(int crc, ByteBuffer b, int[] table) {
- for (; b.remaining() > 7;) {
- final int c0 = (b.get() ^ crc) & 0xff;
- final int c1 = (b.get() ^ (crc >>>= 8)) & 0xff;
- final int c2 = (b.get() ^ (crc >>>= 8)) & 0xff;
- final int c3 = (b.get() ^ (crc >>> 8)) & 0xff;
- crc = (table[0x700 + c0] ^ table[0x600 + c1])
- ^ (table[0x500 + c2] ^ table[0x400 + c3]);
-
- final int c4 = b.get() & 0xff;
- final int c5 = b.get() & 0xff;
- final int c6 = b.get() & 0xff;
- final int c7 = b.get() & 0xff;
-
- crc ^= (table[0x300 + c4] ^ table[0x200 + c5])
- ^ (table[0x100 + c6] ^ table[c7]);
- }
-
- // loop unroll - duff's device style
- switch (b.remaining()) {
- case 7:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- case 6:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- case 5:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- case 4:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- case 3:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- case 2:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- case 1:
- crc = (crc >>> 8) ^ table[((crc ^ b.get()) & 0xff)];
- default: // noop
- }
-
- return crc;
- }
- }
}
Review Comment:
The default `update(byte[], off, len)` delegates to `update(ByteBuffer)` and
(per this interface's contract) expects the passed buffer to be consumed.
However, the only remaining implementation (`ChecksumByteBufferImpl`) does not
advance the position for array-backed buffers on Java 8 (it uses `array()`
without setting `position(limit)`), which can break callers that rely on buffer
consumption semantics. Please update the implementation to always advance the
position (or clarify the contract if consumption is not guaranteed on Java 8).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
