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

Add Crc::digest_with_initial #69

Merged
merged 3 commits into from
Apr 19, 2022
Merged

Conversation

voidc
Copy link
Contributor

@voidc voidc commented Apr 8, 2022

It may sometimes be useful to compute CRC values with an initial value different from the one specified by the used algorithm.
In this case, users currently have to create a new Algorithm for each initial value.
This is particularly problematic if the initial values are not statically known, since Crc::new takes a static reference to an algorithm (see also #68).
With the small change proposed in this PR, an initial value may optionally be supplied when creating a Digest.

@voidc
Copy link
Contributor Author

voidc commented Apr 8, 2022

For reference, the crc32fast crate provides a new_with_initial function.

src/crc8.rs Show resolved Hide resolved
@akhilles
Copy link
Collaborator

Would it be possible to add some test cases with the expected checksums generated by another crc impl? I'm not sure if the way we handle reflection of custom init is considered "correct".

@voidc
Copy link
Contributor Author

voidc commented Apr 13, 2022

I'd say the way it is handled now is conceptually correct since the custom init value is treated in the same way as Algorithm::init which is IMO what users would expect to happen. If you want, I can add a comment to the digest_with_initial methods documenting this behavior. As for tests, I'm not sure what would be a good alternative impl for comparison.

@akhilles
Copy link
Collaborator

I'd say the way it is handled now is conceptually correct since the custom init value is treated in the same way as Algorithm::init which is IMO what users would expect to happen. If you want, I can add a comment to the digest_with_initial methods documenting this behavior. As for tests, I'm not sure what would be a good alternative impl for comparison.

A comment would be enough IMO, and then we can merge.

@akhilles akhilles merged commit 12c17fb into mrhooray:master Apr 19, 2022
@voidc voidc deleted the with-initial branch April 20, 2022 07:21
@voidc
Copy link
Contributor Author

voidc commented Apr 20, 2022

Thanks for merging! Would it be possible to release a new version including this change on crates.io?

@akhilles
Copy link
Collaborator

I'll try to cut a minor release (v2.2.0) by tomorrow. There are some breaking changes that I either need to patch or exclude.

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

Successfully merging this pull request may close these issues.

2 participants