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

Panic instead of Result on to_slice. #43

Open
NickeZ opened this issue Mar 15, 2020 · 4 comments
Open

Panic instead of Result on to_slice. #43

NickeZ opened this issue Mar 15, 2020 · 4 comments
Assignees
Labels

Comments

@NickeZ
Copy link

NickeZ commented Mar 15, 2020

Hey,

I'm curious about the decision to return Result instead of panicking here:

return Err(FromHexError::InvalidStringLength);

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
Copy link
Author

NickeZ commented Mar 15, 2020

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 KokaKiwi self-assigned this Mar 16, 2020
@KokaKiwi
Copy link
Owner

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
Copy link
Author

NickeZ commented Mar 16, 2020

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
Copy link

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.

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

No branches or pull requests

3 participants