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

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

Open Luro02 opened 4 years ago

Luro02 commented 4 years ago

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.