Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Adler32 and CRC32 to Digest #8881

Merged
merged 3 commits into from
Mar 9, 2020

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Mar 5, 2020

Adler32 and CRC32 interface are kept unmodified and hence they don't implement Digest::Base, but their results are 32-bit fixes intead of a slice/string; plus they support the combine alternative. I think the current API while simple is enough.

The top-level versions are deprecated.

@asterite
Copy link
Member

asterite commented Mar 5, 2020

docs_main.cr should be updated as well

@bcardiff
Copy link
Member Author

bcardiff commented Mar 5, 2020

The files are still there. And digest is included with require "./digest/**".

When the deprecated are effectively dropped, yes, the docs_main.cr should be updated.

@straight-shoota
Copy link
Member

I agree on moving Adler32 and CRC32 under a combined namespace. But I'm not sure they fit under the Digest namespace. As far as I am familiar with the matter, a (message) digest is usually associated with a cryptographic hash function (such as MD5 or SHA-1 which are currently available in Digest namespace; OpenSSL::Digest has even more).
Adler32 and CRC32 are not cryptographic hash functions but much weaker checksum/CRC.

We could still continue with this and accept there's some mismatch. Digest is not very specific and already established namespace in Crystal's stdlib.
Alternatively, we could consider a more broader term as top level namespace for hash functions. Hash is already taken, though ;)

@bcardiff
Copy link
Member Author

bcardiff commented Mar 6, 2020

I've found that many std-libs put CRC32 and Adler32 under Digest and none that uses another namespace ...

@asterite
Copy link
Member

asterite commented Mar 6, 2020

golang uses the hash namespace:

That said, I'm fine with digest

@ysbaddaden
Copy link
Contributor

Some digest algorithms are cryptographically strong but that's hardly a requirement, otherwise both MD5 (broken) and SHA-1 (weak) wouldn't be digest algorithms anymore, which would be nonsense.

Don't use CRC32, Adler32, MD5 or SHA-1 for secure usages like verifying that a file wasn't tampered with. Use SHA-2, SHA-3 or BLAKE. But not all usages need to be secure (e.g. gzip, deflate).

This is similar to random number generators: some are them are cryptographically strong.

@ysbaddaden
Copy link
Contributor

Sure we can say hash instead of digest, but the term would be confusing in Crystal (Hash).

@bcardiff bcardiff force-pushed the cleanup/digest-adler32-crc32 branch from 8709d61 to 540e992 Compare March 6, 2020 16:53
@bcardiff
Copy link
Member Author

bcardiff commented Mar 6, 2020

I forgot to change the usages of them in the std-lib.
Rebased on master.

@bcardiff bcardiff force-pushed the cleanup/digest-adler32-crc32 branch from 540e992 to f6763f8 Compare March 6, 2020 18:10
@bcardiff bcardiff merged commit fd96648 into crystal-lang:master Mar 9, 2020
@bcardiff bcardiff added this to the 0.34.0 milestone Mar 9, 2020
@bcardiff bcardiff deleted the cleanup/digest-adler32-crc32 branch March 9, 2020 13:25
@whidbey
Copy link

whidbey commented Apr 10, 2020

i dont notice this change.but i dont' think is good idea to move A to B and move C to D 's breaking change like this.
thoght I admire your commits..but It's a wasting time i think... what I want is better "C" ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants