aherbert commented on issue #27: Murmur3fix URL: https://github.com/apache/commons-codec/pull/27#issuecomment-555263980 All the deprecated `hash32` method can just call `hash32x86`. I think you should drop the new implementation of `hash128`. As I previously suggested you can create a private version that is fixed and call it with the two `h1` and `h2` values. The old broken method will not mask them to unsigned. This makes it very clear that the two implementations are basically the same, just the initialisation of the hashes from the seed is broken. I had a quick test of the methods against Google Guava `com.google.common.hash.Hashing.murmur3_32` and `murmur3_128`. They have chosen to return the values as 16 bytes that are little endian ordered. The output from the the new hash32x86 method matches. Unfortunately (for Google) their `murmur3_128` algorithm has the sign extension error when setting the `h1` and `h2` from the seed. So their implementation matches the broken `hash128` method. I did not do a formal check of the algorithm against [MurmurHash3.cpp](https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp) but checked it by eye. I think this link to the original source should be in the class header. This is the reference for the new implementations, even if adapted from Yonik Seeley. The original MurmurHash3 code has an oddity in that the input bytes and returned output byte hash are read/written using the native byte order of the platform. This is little-endian for x86 and x86-64 intel architecture but nothing mandates that this is done. All the computations are done on 64-bit integers which are logically big endian. Perhaps we should state somewhere that the 128-bit hash is returned as two Java longs. To produce the same output as MurmurHash3.cpp would require endianness conversion as appropriate: ``` long[] hash128 = MurmurHash3.hash128x64("hello world"); byte[] bytes = ByteBuffer.allocate(16) .order(ByteOrder.LITTLE_ENDIAN) .putLong(hash128[0]) .putLong(hash128[1]) .array(); ``` There is little in the Javadoc to explain why things are deprecated. All of the helper `hash32` methods that act on primitives that are deprecated are not broken. This is just for a name change. The broken method is the one that processes byte[]. For hash128 the only method that is broken is one that processes byte[] and does not use the default seed. Sign extension of the default seed does not happen as it is positive. The javadoc should explain that the method is broken when a negative seed is used. If you insist on a new implementation for hash32x86 then it may be more maintainable to copy that to the old `hash32` method and unfix the code to have the bug. Otherwise I would leave hash32 as is and copy it to hash32x86 and fix the sign extension bug. Then add explicit comments in them both above the `// tail` section that this is/is not using the UBYTE_MASK to avoid sign extension in the tail processing. I also still see a `System.out.print` in the test method `doString`. The whitespace formatting from `testCorrectValues_x64` down is all wrong. `IncrementalHash32x86` has no comments on any public methods or the class. It uses raw constants for the final mix step when it can call `fmix32`. Same goes for `hash32x86`. I would replace inlined rotations with `Integer.rotateLeft` for clarity. The aim is to reduce code duplication and make this class more maintainable given that it is supporting the same algorithm with and without bugs. I see no point in having two implementations with variations in their style and code. The code in the two methods should be the same except for the bug. It should be very clear to a future maintainer that the code in `hash32` and `hash32x86` and `IncrementalHash32x86` is the same, save for the bug in the finalisation of `hash32` and the `end` function of `IncrementalHash32`. It should also be clear that `hash128` and `hash128x64` are the same except for the initialisation of the hash from the seed.
---------------------------------------------------------------- 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
