Re: [hash2] Comments

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2024-12-07 00:21:49

On 12/7/24 03:08, Andrey Semashev wrote:
> On 12/7/24 01:42, Ivan Matek wrote:
>>
>>
>> On Fri, Dec 6, 2024 at 10:36 PM Andrey Semashev via Boost
>> <boost_at_[hidden] <mailto:boost_at_[hidden]>> wrote:
>>
>>     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 <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.
>>
>> I was looking into if this could be wrapped into a nicer interface, that
>> is harder to misuse.
> 
> As far as the result() method is concerned, I think it should be renamed
> in the HashAlgorithm interface. I don't see the point in renaming it
> just in the wrapper. The new name should resolve the confusion regarding
> the method purpose and operation.
> 
> As to the usage with the forced move, I don't think the added complexity
> is justified. If the goal is to make sure the hash value doesn't change
> upon successive calls to result() (to use the existing nomenclature)
> then convert it to a pure getter and make a separate finalize() method
> that returns void. If the use case of generation of new values upon
> subsequent result() calls is deemed important then the current interface
> is fine, barring the name of the method. But I don't see the benefit in
> introducing UB (potentially, to be caught by sanitizers - *if* one runs
> one) upon subsequent calls to result().

Though perhaps there is value in prohibiting subsequent calls to
result(), or at least not requiring any specific behavior upon such
calls. One use case is creating wrappers for third party libraries. For
example, in OpenSSL, EVP_DigestFinal_ex prevents any subsequent data to
be consumed by the digest algorithm. If one were to write a Boost.Hash2
wrapper for OpenSSL digest algorithms, it would be difficult to continue
producing data on subsequent calls to result().