Vote for the new std.hash (oops, std.digest)

[Andrei Alexandrescu]

On 8/22/12 8:36 AM, Dmitry Olshansky wrote:
> The discussion around new unified API for digest hash functions has
> subdued, just in time as the review period has ended.
>
> The voting for std.digest package starts today and ends on 29 August.
>
> Rules are simple: reply in this thread with definite "YES" or "NO" on
> whether this package should go into Phobos. Including descriptive short
> notes is appreciated (esp. the NO ones).
>
>
> Latest locations of docs and source:
>
> Code: (location changed!)
> https://github.com/jpf91/phobos/tree/newHash/std/digest
> https://github.com/jpf91/phobos/compare/master...newHash
>
> Docs: (location changed!)
> http://dl.dropbox.com/u/24218791/d/phobos/std_digest_digest.html
> http://dl.dropbox.com/u/24218791/d/phobos/std_digest_md.html
> http://dl.dropbox.com/u/24218791/d/phobos/std_digest_sha.html
> http://dl.dropbox.com/u/24218791/d/phobos/std_digest_crc.html

I couldn't make time for a review during the window, apologies. 
Nevertheless I'd like to make a pass in hope that a few issues can be 
fixed before the code is publicly released.

* No need for start() after default construction in the documentation 
example of CRC32. BTW the documentation of start() suggests that it 
_must_ be called even right after default construction, is that the case?

* In the template interface, no need for void peek(ref ...). Returning 
by value is fine due to the RVO. Same about finish() - just keep the 
functions returning by value.

* This example:

copy(oneMillionRange, &ctx); //Note: You must pass a pointer to copy!

suggests we're doing something wrong. I think a better solution would be 
to have copy() take the target range by "auto ref", and institute this 
passing convention as a general rule for output ranges.

* s/digestType/DigestType/

* s/isDigestable/isDigestible/

* s/startDigest/makeDigest/ - but then again I wonder if requiring 
start() after default construction is needed.

* s/asArray/asStaticArray/. Better yet, we should include this primitive 
in std.conv. Then we get to use to!(ubyte[16])(dynarray).

* The fact that digests aren't unique is trivial by the pigeonhole 
principle, and should not be listed as a bug (the BUGS: tag implies bugs 
in the implementation). It's more interesting if a document can be 
altered to produce a specific digest. If that's what you meant, include 
a link to the appropriate work.

* I don't think the aliases for WrapperDigest!Xyz should be defined. 
Just call that Digest!Xyz and have people use it. It's about as short as 
the aliases.

I vote for inclusion whether or not the points above are addressed, but 
of course it would be great if they were minded before release. 
Apologies for the late review.



Andrei