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

PrimInt: to_be_bytes & co #189

Closed
chrysn opened this issue Oct 1, 2020 · 7 comments · Fixed by #224
Closed

PrimInt: to_be_bytes & co #189

chrysn opened this issue Oct 1, 2020 · 7 comments · Fixed by #224

Comments

@chrysn
Copy link

chrysn commented Oct 1, 2020

The current to_be functions require unsafe to get a slice out of them; the individual types now (since 1.32) have to_be_bytes that does not have that flaw.

I suggest adding to_be_bytes() as well as to_ne_bytes and to_le_bytes as well as the analogous from_be_bytes, from_ne_bytes and from_le_bytes.

To implement this, PrimInt would need to get an associated type that reflects such a buffer; depending on when this gets implemented and what MSRV is targeted then, that could be a const NBYTES: usize (where to_be_bytes would return [u8; NBYTES]), or a BytesBuffer associated type with the right array lengths. In the latter form, the associated type would be required by the trait to implement at least TryFrom<&[u8]> and AsRef<&[u8]>.

Researching into whether that latter form would be upgradable to a const generic one easily later, I found that PrimInt is not sealed; that didn't sound too bad until I noticed that even a default implementation that uses to_be() and then transmutes around could still not come up with a suitable sized and owned return value short of returning a Box<[u8]> (that would technically satisfy the bounds but be ugly). Worse yet, it'd slam the door on being able to migrate towards returning [u8; NBYTES] later (which otherwise could be API compatible as the new return type does satisfy all bounds previously placed on it).

Is there any evolution mechanism in num-traits through which to_be_bytes() could be added?

@cuviper
Copy link
Member

cuviper commented Oct 3, 2020

I don't know any way to extend PrimInt for this.

There could be a new trait though -- #103 was working on this, but it would need to be rebased and reviewed again. Someone can also come up with a new design, if they prefer.

@kaidokert
Copy link
Contributor

Is #103 still the current thinking around it ? If yes, i might want to pick it up and try to resolve pending issues. Related question, if OverflowingShl/Shr implementation like this would also be accepted ?

@kaidokert
Copy link
Contributor

Updated PR

sirhcel added a commit to sirhcel/sensirion-i2c-rs that referenced this issue Dec 28, 2021
The Sensirion SHT4x series uses 8 bit commands and the new function
write_command_u8 has been added to support these devices as well.

For the sake of a clean naming scheme, write_command_u16 is provided as
a replacement for write_command. The latter is still available but
deprecated.

Using suffixes to distinguish between u8 and u16 commands has been
chosen because there is no trait for to_be_bytes which would allow to
use a generic function. The crate num_traits provides the trait PrimInt
which covers to_be but not to_be_bytes. See
rust-num/num-traits#189 for details.
sirhcel added a commit to sirhcel/sensirion-i2c-rs that referenced this issue Dec 29, 2021
The Sensirion SHT4x series uses 8 bit commands and the new function
write_command_u8 has been added to support these devices as well.

For the sake of a clean naming scheme, write_command_u16 is provided as
a replacement for write_command. The latter is still available but
deprecated.

Using suffixes to distinguish between u8 and u16 commands has been
chosen because there is no trait for to_be_bytes which would allow to
use a generic function. The crate num_traits provides the trait PrimInt
which covers to_be but not to_be_bytes. See
rust-num/num-traits#189 for details.
@ctrlcctrlv
Copy link
Contributor

Is this crate simply abandoned? I need the floating point versions of these (to_bits, from_bits specifically, stabilized in 1.20; bytes stabilized in 1.40: rust-lang/rust@7b72c28).

I'm going to just work off of @kaidokert's branch in #224 and make a fork because clearly there's something wrong with maintainership when such an important crate hasn't even allowed workflows to run or even acknowledged #224. I think @kaidokert is far nicer than me, to their credit…and therefore didn't raise this, but I will.

I don't hope to need a fork forever—@hauleth and/or @cuviper need to nominate a new maintainer.

@cuviper
Copy link
Member

cuviper commented Jan 13, 2022

I'm sorry, I will try to get to this, but I have a lot on my plate.

There's also nothing stopping this from being implemented as a separate extension trait, or indeed just using a fork.

@ctrlcctrlv
Copy link
Contributor

No problem, glad to know it's not totally dead :-)

I think you should nominate someone but only you can decide that. I'll maintain a fork for a little while until you're less busy as I need some of the features…

@cuviper cuviper linked a pull request Jul 20, 2023 that will close this issue
bors bot added a commit that referenced this issue Jul 20, 2023
224: Convert between int and bytes #189 r=cuviper a=kaidokert

Picking up #103 from `@flier` to rebase and factor in feedback. Making a quick draft first

Co-authored-by: Kaido Kert <kaidokert@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
Co-authored-by: kaidokert <kaidokert@gmail.com>
@bors bors bot closed this as completed in #224 Jul 20, 2023
@kaidokert
Copy link
Contributor

Woo this finally landed !!! Thank you so much. 🙏

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 a pull request may close this issue.

4 participants