KokaKiwi / rust-hex

A basic crate to encode values to hexadecimal representation. Originally extracted from rustc-serialize.
https://crates.io/crates/hex
Apache License 2.0
201 stars 55 forks source link

Panic instead of Result on `to_slice`. #43

Open NickeZ opened 4 years ago

NickeZ commented 4 years ago

Hey,

I'm curious about the decision to return Result instead of panicking here: https://github.com/KokaKiwi/rust-hex/blob/be0c32f9c8938ca0359bbb0d1477e31b07cb3358/src/lib.rs#L350

Providing the wrong lengths of buffers doesn't seem like a runtime error that one can recover from. It seems more like a logical error by the developer. Adding the overhead of this check and later matching on the result seems unnecessary.

Thanks!

NickeZ commented 4 years ago

When we have const generics we could have this sweetness:

pub fn encode_to_slice<const N: usize, const M: usize>(input: &[u8;N], output: &mut [u8;M]) {
    if N * 2 != M {
        panic!("Invalid output length");
    }

    for (byte, (i, j)) in input.iter().zip(generate_iter(input.len() * 2)) {
        let (high, low) = byte2hex(*byte, HEX_CHARS_LOWER);
        output[i] = high;
        output[j] = low;
    }
}
KokaKiwi commented 4 years ago

According to The Rust Book, maybe we should panic here instead of returning an error, as it can be considered as a "contract violation". But the base thought behind this was that i generally tend to stick to the way libraries should not panic in general and let lib users handle the case, but maybe that's not necessary here idk.

Also, for the const generics stuff, i would prefer having compile-time contracts on const values instead of "runtime" checks on const values :/

NickeZ commented 4 years ago

Yeah me too. I was trying different versions like:

pub fn encode_to_slice<const N: usize>(input: &[u8;N], output: &mut [u8;N*2]) {

But they didn't compile.

In general, yeah, you should propagate errors. But, since I "statically" know that I'm providing the right length of the buffers in my case I will always unwrap on this method. And it seems to me that there are some unnecessary overhead because of this.

edit: and also, when const generics are used I'm pretty sure the conditional would be compiled away, because the whole expression N=2*N is constant.

gquintard commented 4 years ago

Hi, sorry to barge in, I stumble on a related issue: the doc say the output length must be at least twice the inputs, but the code actually needs exactly twice the length. That was confusing.