[hash2] Review Report

From: Matt Borland (matt_at_[hidden])
Date: 2024-12-20 22:02:01

Dear all,

Again, thank you to those who participated in the review of `Hash2` by Peter Dimov and Christian Mazakas from 7 to 15 December 2024.

Bottom line up front: The library is ACCEPTED.

By and large, the discussion around the library was positive, with reviewers appreciating that it implements the "Types don't know #" proposal. I think some discussion was perhaps too narrowly focused on providing hash algorithms, as that is not the over-arching goal of the library. Below is a summary of the main discussion points and recommendations from both reviewers and me.

Discussion and Recommendations:

1) Support for `std::span`. Based on the discussion, it seems like the use of `std::span` has moved from theoretical to being quite popular. Adding an API for span would in principle be OK behind the requisite feature test macros, but it will add to the conceptual requirements which I'm not convinced is beneficial. Unless Peter and Christian intend to move to C++20, and they don't, I think it would be unreasonable at this time to require spans behind macros in the implementation to allow warning-free builds with Clang's `-Wunsafe-buffer-usage` and whatever memory safety profiles turn out to be. If this library was targeting C++20, or intended to be certified by a standards organization, this would be an entirely different conversation.

2) C++ language standard requirement. As I kind of expected, there was limited support for keeping the library at C++11. The authors should feel free to use C++14 and drop support for any non-conforming compilers before initial release.

3) Documentation. Peter Turcan provided a great deal of feedback on the documentation which I expect the authors to handle. There were also quite a few calls to re-arrange the documentation to put working examples and pitfalls discussed during review first, and details of concepts later. The single vs. multi-page documentation debate will never have a clear winner, so I defer to doc writer's preference on that one. The authors have a pretty good sense of the edits they need to make, so I won't belabor the point.

4) Security. Secure construction was included in the conditional acceptance criteria of two reviews - for example, default construction and allowing too-short keys in SipHash were considered harmful. There were also concerns that the algorithms provided would be considered cryptographically secure. Since the library does not intend to be a cryptographic provider (as that is not the real point of the library), I think an annotation at the top of the docs stating as much should be added.

5) Relationship to `ContainerHash`. Several reviewers noted that since there is no explicit Hash1, the relationship with `ContainerHash` should be explained. Peter currently maintains `ContainerHash`, so I think it would be useful to add similar documentation there as well. This could help avoid the situation we have now where `MP11` fully replaces the functionality of `MPL`, but that's not obvious to the casual outside observer.

6) The concept of HashAlgorithm. There was much discussion around the concept of HashAlgorithm and how it deviates from existing practice. If Peter and Christian don't want to be the sole providers of hash algorithms for their library, they need to provide:

 - A way to use existing implementations and/or examples in the documentation (this was part of the acceptance criteria for all but one conditional acceptance). This can be accomplished by adding Guidance and an example of an adapter.

 - An FAQ section addressing questions like "Why does the library impose additional requirements such as result extension on hash algorithms which go beyond the cryptographic specifications in corresponding RFCs?"

Boost.Multiprecision already implements this approach to allow unknown arithmetic backends to be used.

Final Verdict:

I ACCEPT the library. The library was clearly well designed, and the authors provided clear justification for implementation decisions when questioned (e.g., `tag_invoke` for functions as customization points). Peter and Christian are both responsive enough that I expect they will address the above list without needing the forcing function of a conditional accept followed by a review in the future to ensure completeness. In fact Christian already showed me a draft of an OpenSSL adaptor. If this library was coming from an outsider, my opinion would be different. For transparency the final tally of reviews received was ACCEPT: 9, REJECT: 3, CONDITIONAL ACCEPT: 5.

Thanks again to all that participated and congratulations to Peter and Christian.

Happy Holidays,

Matt