On 12/2/24 22:48, Peter Dimov via Boost wrote:
Comments and questions are welcome; you don't need to wait for the review.
A few comments: 1. In the HashAlgorithm interface, there is the result() method. https://pdimov.github.io/hash2/doc/html/hash2.html#hashing_bytes_result I think the name is rather confusing as usually nouns are used for pure accessors, which result() isn't. I think, something like finalize() would be better. At the very least, it should probably be a verb and indicate that the method is actually supposed to modify the internal state before returning the final hash value. I admit that in the docs you describe a use case of repeatedly calling result() to obtain a pseudo-random sequence of numbers, and for that usage the finalize() name doesn't look appropriate. But (a) result() is even less appropriate (because to an uninitiated user it looks like the code always returns the same value) and (b) I would say that use case is not the primary one anyway. 2. Somewhat related, on the use case of extending the size of the hash value by repeatedly calling result(). Is this something that should be encouraged? I'm not a specialist, but my understanding is that extending the hash value this way does not improve its quality, as you get a series of bits in the value that are predetermined based on the lower bits. Isn't that effectively equivalent to just e.g. zero-extending the hash value? I think, if a larger hash value is needed, one should generally use a different hash function with a larger output. If my understanding above is correct then the docs should probably not advise extension by repeatedly calling result() or at the very least have a recommendation to consider using a different hash function in this case. 3. Since there is already the std::hash infrastructure that is supported by std unordered containers and more, have you considered providing means to simplify integration of user types supporting Boost.Hash2 protocol with std::hash? The support could be provided in a number of ways. One example is to provide a separate header with a std::hash specialization that would forward to Boost.Hash2 facilities, if the type supports it. The support could be enabled on opt-in basis e.g. by defining a special member type. Here is how this could be done in C++20: https://godbolt.org/z/1oe51bxPj The user would need to include the boost/hash2/std_hash.hpp header and define a boost_hash2_enable_std_hash member type in his type that supports Boost.Hash2 protocol to automatically obtain a std::hash specialization for it. I'm not sure if this could be done without concepts, but even if not, I think such a utility would be useful. Another idea is to provide a macro that would generate a std::hash specialization for a user's type.