Re: [hash2] Comments

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

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().