Skip to content
This repository was archived by the owner on Apr 19, 2022. It is now read-only.

no_std support for formatting #43

Merged
debris merged 11 commits into from
Aug 3, 2018
Merged

no_std support for formatting #43

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9183976
adds no std support
Pzixel Jul 20, 2018
6fc5c22
typos
Pzixel Jul 23, 2018
8753e8c
overflow issue
Pzixel Jul 23, 2018
3188c16
buffer fix
Pzixel Jul 23, 2018
2cba077
Fixes quickcheck
Pzixel Jul 23, 2018
817ea95
typo
Pzixel Jul 23, 2018
d91191d
Removes extra cast
Pzixel Jul 23, 2018
b7817e5
Returns `From` trait (it's better to create a separate PR). Adds fmt …
Pzixel Jul 23, 2018
024a0fd
Fixes tests
Pzixel Jul 23, 2018
84aa058
Fixes naming issue
Pzixel Jul 23, 2018
fca8052
Fixes formatting
Pzixel Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 24 additions & 30 deletions src/uint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ macro_rules! construct_uint {
use core::cmp;
use rustc_hex::ToHex;;

if self.is_zero() { return "0".to_owned(); } // special case.
if self.is_zero() { return "0".to_owned(); } // construct_uintspecial case.

@debris debris Jul 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be reverted or there should be a space between uint and special

@Pzixel Pzixel Jul 22, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be reverted. Just a typo.

let mut bytes = [0u8; 8 * $n_words];
self.to_big_endian(&mut bytes);
let bp7 = self.bits() + 7;
Expand Down Expand Up @@ -910,34 +910,35 @@ macro_rules! construct_uint {
}
}

#[cfg(feature="std")]
impl ::core::fmt::Debug for $name {
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
::core::fmt::Display::fmt(self, f)
}
}

#[cfg(feature="std")]
impl ::core::fmt::Display for $name {
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
if self.is_zero() {
return write!(f, "0");
}

let mut s = String::new();
let mut buf = [0_u8, $n_words*8];
let mut i = buf.len() - 1;
let mut current = *self;
let ten = $name::from(10);

while !current.is_zero() {
s = format!("{}{}", (current % ten).low_u32(), s);
let digit = (current % ten).low_u32() as u8;
buf[i] = digit + b'0';
current = current / ten;
i -= 1;
}

write!(f, "{}", s)
let s = unsafe {::core::str::from_utf8_unchecked(&buf[i..])};

@debris debris Jul 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unjustified unsafe

@Pzixel Pzixel Jul 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String of numbers is guaranteed to be a valid UTF8 string. I can leave a comment though.

f.write_str(s)
}
}

#[cfg(feature="std")]
impl ::core::fmt::LowerHex for $name {
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
let &$name(ref data) = self;
Expand All @@ -955,13 +956,6 @@ macro_rules! construct_uint {
Ok(())
}
}

#[cfg(feature="std")]
impl From<&'static str> for $name {

@debris debris Jul 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no reason to remove this

@Pzixel Pzixel Jul 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you cannot convert into type without panic then you shoulnd't implement From.

https://doc.rust-lang.org/std/convert/trait.From.html

Note: this trait must not fail.

@debris debris Jul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a convenience wrapper used only in tests (that's why it requires 'static lifetime). We can argue here whether it's a good practice or not, but I won't accept this change cause it is used thousands of times in tests utilizing this library.

@Pzixel Pzixel Jul 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then it could be hidden under #cfg[test]? Or it's better to provide some helper trait, if you want to save 6 chars.

@NikVolf NikVolf Jul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

panic is not fail, it's panic (unlike typed Result<T, E> which is fail)

[cfg(test)] does not propagate through dependencies, and those tests mentioned are not inside this repo exclusively

and anyway, @Pzixel , how is this related to the pr topic?

@Pzixel Pzixel Jul 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

panic is not fail, it's panic (unlike typed Result<T, E> which is fail)

I'm pretty sure that this quote says conversion must always succeed. If you disagree, what do you think they added this note in the documentation and what does it mean? I'd really like ot make my code better, and such a basic concept worth discussing it.

how is this related to the pr topic?

I was looking at all things with std attribute to deretmine if they could work in no-std environment. And then I realised that this method couldn't work anywhere.

I probably should create another PR for it, but I strongly think that it should be done. I understand that in may require changes in depended crates, but it's really goes against rust guidelines (as I understand the docs).

@mkpankov mkpankov Jul 23, 2018 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@debris @NikVolf guys, if you need a convenience wrapper used only in tests, you can make any wrapper you'd like, just call it parse_unwrap or anything else, not an ecosystem-wide method with defined semantics.
There's nothing preventing a user of your library from calling .into() that would unexpectedly fail.

@NikVolf NikVolf Jul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkpankov not unexpectedly, because it will fail always or not fail always (since there is no dynamic behaviour, input is static)

fn from(s: &'static str) -> Self {
s.parse().unwrap()
}
}
);
}

Expand Down Expand Up @@ -1997,25 +1991,25 @@ mod tests {
let (result, _) = U256([1, 0, 0, 0]).overflowing_mul(U256([0, 0, 0, u64::max_value()]));
assert_eq!(U256([0, 0, 0, u64::max_value()]), result);

let x1: U256 = "0000000000000000000000000000000000000000000000000000012365124623".into();
let x2sqr_right: U256 = "000000000000000000000000000000000000000000014baeef72e0378e2328c9".into();
let x1: U256 = "0000000000000000000000000000000000000000000000000000012365124623".parse().unwrap();
let x2sqr_right: U256 = "000000000000000000000000000000000000000000014baeef72e0378e2328c9".parse().unwrap();
let x1sqr = x1 * x1;
assert_eq!(x2sqr_right, x1sqr);

let x1cube = x1sqr * x1;
let x1cube_right: U256 = "0000000000000000000000000000000001798acde139361466f712813717897b".into();
let x1cube_right: U256 = "0000000000000000000000000000000001798acde139361466f712813717897b".parse().unwrap();
assert_eq!(x1cube_right, x1cube);

let x1quad = x1cube * x1;
let x1quad_right: U256 = "000000000000000000000001adbdd6bd6ff027485484b97f8a6a4c7129756dd1".into();
let x1quad_right: U256 = "000000000000000000000001adbdd6bd6ff027485484b97f8a6a4c7129756dd1".parse().unwrap();
assert_eq!(x1quad_right, x1quad);

let x1penta = x1quad * x1;
let x1penta_right: U256 = "00000000000001e92875ac24be246e1c57e0507e8c46cc8d233b77f6f4c72993".into();
let x1penta_right: U256 = "00000000000001e92875ac24be246e1c57e0507e8c46cc8d233b77f6f4c72993".parse().unwrap();
assert_eq!(x1penta_right, x1penta);

let x1septima = x1penta * x1;
let x1septima_right: U256 = "00022cca1da3f6e5722b7d3cc5bbfb486465ebc5a708dd293042f932d7eee119".into();
let x1septima_right: U256 = "00022cca1da3f6e5722b7d3cc5bbfb486465ebc5a708dd293042f932d7eee119".parse().unwrap();
assert_eq!(x1septima_right, x1septima);
}

Expand All @@ -2028,7 +2022,7 @@ mod tests {

#[test]
fn little_endian() {
let number: U256 = "00022cca1da3f6e5722b7d3cc5bbfb486465ebc5a708dd293042f932d7eee119".into();
let number: U256 = "00022cca1da3f6e5722b7d3cc5bbfb486465ebc5a708dd293042f932d7eee119".parse().unwrap();
let expected = [
0x19, 0xe1, 0xee, 0xd7,
0x32, 0xf9, 0x42, 0x30,
Expand Down Expand Up @@ -2111,7 +2105,7 @@ mod tests {

#[test]
fn fixed_arrays_roundtrip() {
let raw: U256 = "7094875209347850239487502394881".into();
let raw: U256 = "7094875209347850239487502394881".parse().unwrap();
let array: [u8; 32] = raw.into();
let new_raw = array.into();

Expand Down Expand Up @@ -2148,18 +2142,18 @@ mod tests {

#[test]
fn leading_zeros() {
assert_eq!(U256::from("000000000000000000000001adbdd6bd6ff027485484b97f8a6a4c7129756dd1").leading_zeros(), 95);
assert_eq!(U256::from("f00000000000000000000001adbdd6bd6ff027485484b97f8a6a4c7129756dd1").leading_zeros(), 0);
assert_eq!(U256::from("0000000000000000000000000000000000000000000000000000000000000001").leading_zeros(), 255);
assert_eq!(U256::from("0000000000000000000000000000000000000000000000000000000000000000").leading_zeros(), 256);
assert_eq!("000000000000000000000001adbdd6bd6ff027485484b97f8a6a4c7129756dd1".parse::<U256>().unwrap().leading_zeros(), 95);

@debris debris Jul 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would you change this? imo it's less readable

@Pzixel Pzixel Jul 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because into shouldn't panic. Current implementation cannot convert any string into U256. This is what parse is for. See comment below.

assert_eq!("f00000000000000000000001adbdd6bd6ff027485484b97f8a6a4c7129756dd1".parse::<U256>().unwrap().leading_zeros(), 0);
assert_eq!("0000000000000000000000000000000000000000000000000000000000000001".parse::<U256>().unwrap().leading_zeros(), 255);
assert_eq!("0000000000000000000000000000000000000000000000000000000000000000".parse::<U256>().unwrap().leading_zeros(), 256);
}

#[test]
fn trailing_zeros() {
assert_eq!(U256::from("1adbdd6bd6ff027485484b97f8a6a4c7129756dd100000000000000000000000").trailing_zeros(), 92);
assert_eq!(U256::from("1adbdd6bd6ff027485484b97f8a6a4c7129756dd10000000000000000000000f").trailing_zeros(), 0);
assert_eq!(U256::from("8000000000000000000000000000000000000000000000000000000000000000").trailing_zeros(), 255);
assert_eq!(U256::from("0000000000000000000000000000000000000000000000000000000000000000").trailing_zeros(), 256);
assert_eq!("1adbdd6bd6ff027485484b97f8a6a4c7129756dd100000000000000000000000".parse::<U256>().unwrap().trailing_zeros(), 92);
assert_eq!("1adbdd6bd6ff027485484b97f8a6a4c7129756dd10000000000000000000000f".parse::<U256>().unwrap().trailing_zeros(), 0);
assert_eq!("8000000000000000000000000000000000000000000000000000000000000000".parse::<U256>().unwrap().trailing_zeros(), 255);
assert_eq!("0000000000000000000000000000000000000000000000000000000000000000".parse::<U256>().unwrap().trailing_zeros(), 256);
}

mod laws {
Expand Down