Re: [hash2] Formal Review Begins

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2024-12-07 15:27:08

This is my review of boost hash2.

Since I encourage people to write smaller reviews, here's mine. Please
weigh it accordingly.

I cloned the repo and ran the tests, all worked.
I only studied the docs (not the implementation). If there's one person
who's implementation I blindly trust, it's Peter.
That plus writing the review took about an hour.

The choice of hash algorithms looks correct to me..

HashAlgorithm should allow me to plugin any other algorithm, e.g. from
openssl.

I also think that update(void*,size_t) is the correct interface, because it
doesn't require a reinterpret_cast to an inappropriate type,
nor does it create any dependency.

The built-in algorithms for hashing are a good choice although I wonder
about the usefulness of pointers. A pointer doesn't really tell if it's
itself information or points to it (what do you hash for const char*?)
I would limit it to void* and function pointers. However, many other hash
libraries have it built-in too. Following precedent here seems ok to me.

Another example that I would be interested in is how one would hash a
`boost::json::value`. If it's just calling `hash_append_unordered_range`,
can recursion become an issue? If so, how do I address it?

I think this library does solve multiple issues with std::hash/boost.hash:`

 1. having an actual hash algorithm
 2. ability to choose a hash algorithm
 3. convenient extensibility
 4. endian awareness

I think (1) is the most important, because with std::hash everyone needing
his class to be hashed must know how to implement/combine hashes without
knowing which hash algorithm is used under neath.
By using an established algorithm and just being required to provide the
bytes to be hashed, the user does not need to know anything about hashing
algorithms.

It is also very useful as is, e.g. I can drop it into a project using
unordered_map by just replacing the `Hash` parameter.

I think the design of the library is correct and recommend to ACCEPT this
library into boost.

On Sat, Dec 7, 2024 at 9:47 PM Matt Borland via Boost <boost_at_[hidden]>
wrote:

> Dear all,
>
> The review of Hash2 by Peter Dimov and Christian Mazakas begins today
> Saturday, December 7th through Sunday December 15th, 2024.
>
> Code: https://github.com/pdimov/hash2
> Docs: https://pdimov.github.io/hash2/
>
> Please provide feedback on the following general topics:
>
> - What is your evaluation of the design?
> - What is your evaluation of the implementation?
> - What is your evaluation of the documentation?
> - What is your evaluation of the potential usefulness
> of the library? Do you already use it in industry?
> - Did you try to use the library? With which compiler(s)? Did
> you have any problems?
> - How much effort did you put into your evaluation?
> A glance? A quick reading? In-depth study?
> - Are you knowledgeable about the problem domain?
>
> Ensure to explicitly include with your review: ACCEPT, REJECT, or
> CONDITIONAL ACCEPT (with acceptance conditions).
>
> There's already been a flurry of conversation so I thank you all for that.
> If you have any questions during this process feel free to reach out to me.
> Additionally, if you would like to submit your review privately, which I
> will anonymize for the review report, you may send it directly to me at:
> matt_at_mattborland.com.
>
> Matt Borland
> Review Manager and C++Alliance Staff Engineer
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>