[r2r] safe number type casting #1517
Conversation
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
mm2src/common/number_type_casting.rs
Outdated
| impl_safe_number_type_cast!(u128, u8); | ||
|
|
||
| // I128 | ||
| impl_safe_number_type_cast!(i128, i64); |
There was a problem hiding this comment.
Is impl_safe_number_type_cast!(i128, isize); missing?
Signed-off-by: Onur Özkan <work@onurozkan.dev>
| ($from: ident, $to: ident) => { | ||
| impl SafeTypeCastingNumbers<$to> for $from { | ||
| fn into_or(self, or: $to) -> $to { std::convert::TryFrom::try_from(self).unwrap_or(or) } | ||
| fn into_or_max(self) -> $to { std::convert::TryFrom::try_from(self).unwrap_or(std::$to::MAX) } |
There was a problem hiding this comment.
I find into_or_max a bit confusing for signed -> unsigned cast like impl_safe_number_type_cast!(i128, u64);. IMHO, it won't have a realistic use case. For example, if we get a negative value for a number supposed to represent an amount of money (e.g. BTC satoshis), we would likely prefer to return an error in such case instead of using max possible value. In general, should we maybe consider using TryFrom/TryInto directly and use unwrap_or where applicable?
There was a problem hiding this comment.
I find into_or_max a bit confusing for signed -> unsigned cast like
impl_safe_number_type_cast!(i128, u64); so we will be able to use into_or_zero or into_or, there we should not use into_or_max.
For example, if we get a negative value for a number supposed to represent an amount of money (e.g. BTC satoshis), we would likely prefer to return an error in such case instead of using max possible value.
Then if we use this implementation, we should also add function that takes closure as an argument and uses it if try_from fails.
In general, should we maybe consider using TryFrom/TryInto directly and use unwrap_or where applicable?
For me it's also fine. Anything that replaces wrong x as y castings is fine enough :)
There was a problem hiding this comment.
I changed my point of view, so I agree with Artem about this. It's better to handle negative values manually.
I find into_or_max a bit confusing for signed -> unsigned cast like impl_safe_number_type_cast!(i128, u64);.
There was a problem hiding this comment.
I changed my point of view, so I agree with Artem about this. It's better to handle negative values manually.
I guess we should close the PR then?
There was a problem hiding this comment.
No, we still need to cast unsigned to signed in my opinion
There was a problem hiding this comment.
@artemii235 any comments? If you aggree with Sergey, I can do that.
There was a problem hiding this comment.
Yes, I agree - let's keep unsigned -> signed cast and handle negative values manually.
There was a problem hiding this comment.
What about unsigned -> unsigned casting(like u64 to u16 might cause overflow and produce random value) ? @artemii235 @sergeyboyko0791
There was a problem hiding this comment.
I think it'll be helpful!
What about unsigned -> unsigned casting
There was a problem hiding this comment.
Yeah, having e.g. into_or_max seems reasonable for such cases.
Signed-off-by: ozkanonur <work@onurozkan.dev>
|
Please review again @artemii235 @sergeyboyko0791 Btw, I think |
Signed-off-by: ozkanonur <work@onurozkan.dev>
|
@sergeyboyko0791 Could you take one more look at this PR, please? |
I think we should replace all the possible overflow cases with this implementation. Should we cover that in this PR? @artemii235
Atm it only covers this line because @sergeyboyko0791 specifically asked for it here.
Opened from: #1515 (comment)
Resolves: #1500