Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add back to/from i32 fns #765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattsse
Copy link

@mattsse mattsse commented Nov 20, 2024

in #743 these have been removed in favor of tryfrom/from traits.

this was a breaking change, the into 32 makes it a bit weirder to convert it to an u8. id.to_i32() as u8 is now i32::from(id) as u8.

imo it's reasonable to add them back, so this doesn't break for everyone that hasn't updated yet.

@tcharding
Copy link
Member

Thanks for the contribution. Rust convention is to use From and TryFrom for conversions. I do not think we should add from_i32 back in.

to_i32() however seems useful and IMO probably should not have been removed, we could also add to_u8 if going to a u8 is common.

Both functions can be backported to v0.30.0.

@apoelstra
Copy link
Member

Agreed re from_i32 -- I think we should add it back but deprecate it.

Also agreed that we should add a to_u8 alongside to_i32. This value fits into a u8 and you probably want it in a u8 if your intent is to serialize it.

@apoelstra
Copy link
Member

Indeed, in rust-bitcoin where we use this function we have casts in both directions...when serializing, we do to_i32() as u8 which sucks, and also when deserializing we use as i32 to convert a u8 to a i32, which we shouldn't be doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants