aherbert commented on issue #27: Murmur3fix URL: https://github.com/apache/commons-codec/pull/27#issuecomment-554589175 There is a lot of new code in here. IIUC the only method that was broken was the hash32 variant when the input had a number of bytes that was not a factor of 4. This was due to a missing mask to perform sign extension when processing the final 3 bytes. So actually for 1/4 cases of random byte lengths and 1/2 cases for strings it works anyway. I think the 64-bit and 128-bit versions are not broken as masks are used in the final processing of the last bytes to convert byte to integer then cast to long: ``` @Test public void testShift() { for (int i = 0; i < 256; i++) { byte bi = (byte) i; for (int j = 0; j < 8; j++) { int shift = j * 8; // Current method long shift1 = (long) (bi & 0xff) << shift; // Your new version long shift2 = (bi & 0xffL) << shift; Assert.assertEquals(shift1, shift2); } } } ``` Why deprecate the 128-bit version and provide a whole new implementation that you have copied from elsewhere. Do the two methods produce the same results? Can you call both of them with a few thousand arrays of random bytes with different lengths. If so then this is just bringing in a new implementation with a different name (hash128 -> hash128_x64). All of the convenience methods for hash32 are not broken. An int and a long already have a factor of 4 for the bytes. The new methods are just name changes and exactly the same code. So you have a lot of duplication and no need for deprecation other than a name change. I think the only methods to be deprecated are: ``` public static int hash32(final byte[] data, final int offset, final int length, final int seed) (plus the 4 methods that call it) IncrementalHash32.end() (this can be deprecated in favour of `.end32x86()`) ``` You have added a hash for a CharSequence. Although this would be useful it is not in the scope of this PR which is fixing the broken 32-bit hash implementation. Am I missing something here?
---------------------------------------------------------------- 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] With regards, Apache Git Services
