aherbert commented on issue #27: Murmur3fix URL: https://github.com/apache/commons-codec/pull/27#issuecomment-555420876 I've been considering the new method `hash32x86(CharSequence data, ...` I do not think that we should enforce the UTF-8 encoding on the char sequence. A char is a 16-bit unsigned integer. A simpler approach would be to encode the chars as unsigned 16-bit integers. This can process blocks of data just as is done for the byte[] but in blocks of size 2 and not 4. Removing all the if statements and just blindly encoding everything would be simpler. How this would effect most cases of ASCII text where the top 8 bits will be empty I do not know. If a user would like to hash using a character set then they should convert to bytes and encode that. IMO the character set is not relevant. String.equals(Object) treats two Strings as equal if they have the same character in each position. The character set does not matter. It is a binary data comparison. I think hashing should be a binary data operation. It does not care what the data represents (i.e. the character set). The way Guava handles adding a `char` to a hasher is exactly this. It just puts in 2 * 8-bit bytes for each char. The AbstractHasher has these methods: ``` @Override public Hasher putUnencodedChars(CharSequence charSequence) { for (int i = 0, len = charSequence.length(); i < len; i++) { putChar(charSequence.charAt(i)); } return this; } @Override public Hasher putString(CharSequence charSequence, Charset charset) { return putBytes(charSequence.toString().getBytes(charset)); } ``` So you either add chars as unencoded or you specify a character set which uses a byte conversion. This makes me think we should change `hashXXX(CharSequence, ...)` to be documented as processing uncoded 16-bit chars and not add new `hashXXX(String [, ...])` methods which are vague. The originals will have to stay but deprecated with an appropriate message. The choice of charsets can be left to the user to convert and use the appropriate byte[] methods. If you prefer we can drop new methods with String and CharSequence and discuss on the mailing list. This PR can just fix the `byte[]` implementations and clarify naming conventions for the methods.
---------------------------------------------------------------- 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
