feat(crc): add crc hash fn #1136
Conversation
src/stdlib/crc32.rs
Outdated
| fn parameters(&self) -> &'static [Parameter] { | ||
| &[Parameter { | ||
| keyword: "value", | ||
| kind: kind::ANY, |
There was a problem hiding this comment.
Hm I noticed this in other hash function. I don't think ANY is ideal but we can leave it here for consistency.
|
Awesome @pront. I have created a PR on vector to document this function |
src/stdlib/crc32.rs
Outdated
There was a problem hiding this comment.
There are many kinds of CRC functions, even many kinds of 32-bit CRC functions. Instead of adding a separate function for each, would it make sense to instead have a single crc function with some "algorithm" parameter like decrypt, encrypt, and hmac use?
There was a problem hiding this comment.
Good point. @ivor11 should be trivial to modify this PR to do the above. Let me know if this makes sense.
There was a problem hiding this comment.
Awesome, thanks. I will take a look at the updated code.
| use crate::value; | ||
| use crc::Crc as CrcInstance; | ||
|
|
||
| fn crc(value: Value, algorithm: Value) -> Resolved { |
There was a problem hiding this comment.
💭 This is one of those (rare) cases where a #[proc_macro] can significantly reduce the boilerplate. We could iterate over valid_algorithms and keep pushing:
$algo => CrcInstance::<u8>::new(&crc::$algo)
.checksum(&value)
.to_string(),
But this is just my observation. Feel free to ignore this comment.
| tdef: TypeDef::bytes().infallible(), | ||
| } | ||
| ]; | ||
| } |
There was a problem hiding this comment.
can you add a test case with an invalid algorithm?
src/stdlib/crc.rs
Outdated
| } | ||
|
|
||
| fn type_def(&self, state: &state::TypeState) -> TypeDef { | ||
| let valid_algorithms = [ |
There was a problem hiding this comment.
Let's move this at the top as a const VALID_ALGORITHMS: &[&str] = &[
src/stdlib/crc.rs
Outdated
| "CRC_82_DARC", | ||
| ]; | ||
|
|
||
| let mut valid_static_algo = false; |
There was a problem hiding this comment.
I think we can simplify this a little bit:
fn type_def(&self, state: &state::TypeState) -> TypeDef {
if let Some(algorithm) = self.algorithm.as_ref() {
if let Some(algorithm) = algorithm.resolve_constant(state) {
if let Ok(algorithm) = algorithm.try_bytes_utf8_lossy() {
let algorithm = algorithm.to_uppercase();
if VALID_ALGORITHMS.contains(&algorithm.as_str()) {
return TypeDef::bytes().infallible();
}
}
}
} else {
return TypeDef::bytes().infallible();
}
TypeDef::bytes().fallible()
}
(as long as the tests are passing)
|
This also need a changelog to communicate this new feature to the users. |
|
@pront my bad, I've added the changelog file. Thanks for the thorough review |
Summary
Add crc hash fn with an algorithm parameter
default algorithm used - CRC_32_ISO_HDLC
Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
our guidelines.
Checklist
run
dd-rust-license-tool writeand commit the changes. More details here.References
#997