Add VarInt from implementations by way of macro #2024
Conversation
Throughout the codebase we cast values to `u64` when constructing a `VarInt`. We can make the code marginally cleaner by adding `From<T>` impls for all unsigned integer types less than or equal to 64 bits. Also allows us to (possibly unnecessarily) comment the cast in a single place.
|
If this is agreeable we could use the same idea (along with custom traits when necessary) for any constructor in our codebase that takes a EDIT: hmm maybe using a custom trait is too burdensome on users (importing the trait)? |
|
@tcharding I like this idea for |
|
Just checking it was clear from what I wrote before, I didn't mean adding |
| @@ -436,26 +451,26 @@ impl Decodable for VarInt { | |||
| if x < 0x100000000 { | |||
| Err(self::Error::NonMinimalVarInt) | |||
| } else { | |||
| Ok(VarInt(x)) | |||
| Ok(VarInt::from(x)) | |||
There was a problem hiding this comment.
nit this does not seem to be a cast here.
|
It seems like a lot of code to change:
Interesting idea though. I suppose this could help future proof for accidentally truncating u128 as u64 if that's a thing that could happen. |
|
I don't really see what the added value is/was for this MR.. I don't see how the new version is better. On several occasions I have thought the If anyone likes this, I could try this out. |
|
@stevenroose yeah, that may be the better approach here. We may even be able to genericize it so there was a trait that applied to all numeric types allowing them to be |
Did this here: #2133 |
Throughout the codebase we cast values to
u64when constructing aVarInt. We can make the code marginally cleaner by addingFrom<T>impls for all unsigned integer types less than or equal to 64 bits. Also allows us to (possibly unnecessarily) comment the cast in a single place.