no_std support for formatting#43
Conversation
|
Btw tests doesn't pass in |
debris
left a comment
debris
left a comment
There was a problem hiding this comment.
It seems that you abuse fact that compiler doesn't check for overflow in release
I don't understand this. So why they suddenly started failing for your branch and they work on master?
| 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); |
There was a problem hiding this comment.
why would you change this? imo it's less readable
There was a problem hiding this comment.
Because into shouldn't panic. Current implementation cannot convert any string into U256. This is what parse is for. See comment below.
| } | ||
|
|
||
| #[cfg(feature="std")] | ||
| impl From<&'static str> for $name { |
There was a problem hiding this comment.
there is no reason to remove this
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@mkpankov not unexpectedly, because it will fail always or not fail always (since there is no dynamic behaviour, input is static)
| } | ||
|
|
||
| write!(f, "{}", s) | ||
| let s = unsafe {::core::str::from_utf8_unchecked(&buf[i..])}; |
There was a problem hiding this comment.
String of numbers is guaranteed to be a valid UTF8 string. I can leave a comment though.
| 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. |
There was a problem hiding this comment.
it should be reverted or there should be a space between uint and special
There was a problem hiding this comment.
this will be reverted. Just a typo.
|
|
Now also fixes #1 (MAX was useful to test against). |
Pzixel
changed the title
|
looks good enough now to me, @debris ? |
|
isn't it a bit outdated for the nightly though? |
|
All tests pass on 2018-08-01 |
What do you mean? |
4 participants
Includes parsing fixes as well.
Fixes #31
Fixes #1