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

Is FromIterator the correct return type for ToHex/FromHex? #37

Open
Luro02 opened this issue Dec 29, 2019 · 0 comments
Open

Is FromIterator the correct return type for ToHex/FromHex? #37

Luro02 opened this issue Dec 29, 2019 · 0 comments
Projects
Milestone

Comments

@Luro02
Copy link
Contributor

Luro02 commented Dec 29, 2019

The problematic parts of code:

fn encode_to_iter<T: iter::FromIterator<char>>(table: &'static [u8; 16], source: &[u8]) -> T {
    BytesToHexChars::new(source, table).collect()
}

impl<T: AsRef<[u8]>> ToHex for T {
    fn encode_hex<U: iter::FromIterator<char>>(&self) -> U {
        encode_to_iter(HEX_CHARS_LOWER, self.as_ref())
    }

    fn encode_hex_upper<U: iter::FromIterator<char>>(&self) -> U {
        encode_to_iter(HEX_CHARS_UPPER, self.as_ref())
    }
}

By returning FromIterator you are forcing the function user, to collect the data into something (most likely something from alloc like String or Vec).

I would propose to change the bounds to IntoIterator<Item = char>, you could still collect it into for example a String
value.encode_hex().collect::<String>();, but with the benefit of being able to loop though the entire thing without extra allocations.

For example #8 needs a wrapper type, that implements fmt::UpperHex, with the current definition you would have to write it like this

use crate::ToHex;
use core::fmt;

pub struct Hex<T>(pub T);

impl<T: ToHex> fmt::UpperHex for Hex<T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{}", self.0.encode_hex_upper::<String>())
    }
}

As you can see you have to create a temporary String, which would require alloc.

If the trait would return IntoIterator<Item = char> you could do this

use crate::ToHex;
use core::fmt;

pub struct Hex<T>(pub T);

impl<T: ToHex> fmt::UpperHex for Hex<T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
       for c in self.0.encode_hex_upper() {
              write!(f, "{}", c)?;
       }
       Ok(())
    }
}

which does not allocate anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v1.0
  
To do
Development

No branches or pull requests

2 participants