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

Convert between int and bytes #189 #224

Merged
merged 9 commits into from
Jul 20, 2023

Conversation

kaidokert
Copy link
Contributor

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

@kaidokert kaidokert force-pushed the convert_between_int_and_bytes branch from 86709c0 to 60482c1 Compare October 17, 2021 20:46
@kaidokert
Copy link
Contributor Author

Hm, i get the rest of the builds green, but no idea how to make doctests not to break with 1.8, because attributes on non-item statements and expressions are experimental. (see issue #15701)

@kaidokert kaidokert force-pushed the convert_between_int_and_bytes branch 2 times, most recently from 99ea5e8 to 56b1d2f Compare October 18, 2021 02:32
@kaidokert kaidokert marked this pull request as ready for review October 18, 2021 02:39
@kaidokert
Copy link
Contributor Author

Ok i got all doctests to pass here ( sample run ) although dealing with 1.8 is very clunky.

@kaidokert
Copy link
Contributor Author

Ping - any chance of unblocking the workflows ?

@ctrlcctrlv
Copy link
Contributor

Is this crate simply abandoned?

@ctrlcctrlv
Copy link
Contributor

(It's not: see #189 (comment))

@ShaneMurphy2
Copy link

ShaneMurphy2 commented Jan 13, 2022

Tried out the PR directly from the branch, ergonomics is a bit weird where calling to_be_bytes then passing the return value to a function expecting a byte slice complains that it found the associated type instead of &[u8]. You essentially have to call .as_ref() to use the value where a slice is expected.
image

@kaidokert
Copy link
Contributor Author

Ping for @hauleth, perhaps you'd be able to unblock the CI checks ?

@kaidokert kaidokert force-pushed the convert_between_int_and_bytes branch from 56b1d2f to 2b6bcbe Compare May 14, 2022 17:34
@kaidokert
Copy link
Contributor Author

Rebased on master again - checks are green.

@kaidokert
Copy link
Contributor Author

Is there any interest still in landing this ? I can rebase/reintegrate if yes. Would be really nice to get at least checks ran

@cuviper
Copy link
Member

cuviper commented Oct 19, 2022

Yeah, sorry, go ahead and rebase and I'll review it.

Note, the master branch now requires Rust 1.31, so I think to/from_bits will ease the fallback float implementation.

@kaidokert kaidokert force-pushed the convert_between_int_and_bytes branch from 2b6bcbe to 7265426 Compare October 22, 2022 01:44
@kaidokert
Copy link
Contributor Author

Rebased, had to end up squashing.

So to/from_le_bytes for integer types unfortunately only exists since 1.32 and floats from 1.40, so that doesn't actually clean up the implementation.

to/from_bits don't seem to directly help as it can convert u32->f32 but not a [u8;1] to f32

@cuviper
Copy link
Member

cuviper commented Oct 22, 2022

You already have a decent fallback for integers, so what I meant was that the float fallback can be something like self.to_bits().to_le_bytes() and Self::from_bits(<$I as ToFromBytes>::from_le_bytes(&bytes)). That doesn't need to care whether the integer part is using the native version or fallback.

(And if you peek into the standard library, that's basically how the native float methods are implemented!)

@kaidokert
Copy link
Contributor Author

Thanks - yes that worked better. Please have another look.

@kaidokert
Copy link
Contributor Author

Periodic ping for this PR. Would be really nice if the workflows could be allowed run.

@cuviper
Copy link
Member

cuviper commented Feb 10, 2023

Hmm, I don't see the workflow button, but let me try pushing a rebase...

@cuviper cuviper force-pushed the convert_between_int_and_bytes branch from 5c0f252 to b156801 Compare February 10, 2023 22:27
@cuviper
Copy link
Member

cuviper commented Feb 10, 2023

The implementation looks good. I'm now thinking more about the API design, and maybe we should split it up, like:

  • trait ToBytes for to_*_bytes methods
  • trait FromBytes for from_*_bytes methods
  • trait NumBytes as an empty meta-trait covering the constraints you have on type Bytes, and a blanket impl.

The main reason to split To/From is because we could add To implementations for NonZeroU32 et al, but those can't safely implement From unless we make them panic on 0.

What do you think?

@kaidokert
Copy link
Contributor Author

Thanks for taking a look and sorry for long response. Yes, splitting it up looks like a good idea, pushed the change with last commit here. Unsure if NumBytes should be public/exported for anything ?

Please have another look ( workflows pass ).

@blakehawkins
Copy link

I would also find this useful!

@cuviper
Copy link
Member

cuviper commented Apr 13, 2023

After trying this with num-bigint, I figured FromBytes::Bytes should allow unsized so that can be [u8] in that case. I also added default impls for the to/from_ne_bytes based on target_endian, as that seems to be convenient. Let me know what you think!

(I also changed a few other things in the implementation, not affecting API.)

@kaidokert
Copy link
Contributor Author

Those all look like great additions and clean-ups to me, API is much better. Thanks for those.

Is there anything else that needs to be done before merging ?

@rdaum
Copy link

rdaum commented Jun 22, 2023

I've been using this feature ( through manual commit hash in Cargo.toml) as part of a datastructure I've written (e.g. https://github.com/rdaum/rart-rs/blob/main/rart/src/keys/array_key.rs#L25), and wondering what the status on it is? If there's no reasonable prospect of it getting merged, I will have to look for another solution, but was hoping to see this PR land.

If there's any testing, etc. I can help with, let me know.

@cuviper cuviper linked an issue Jul 20, 2023 that may be closed by this pull request
@cuviper cuviper force-pushed the convert_between_int_and_bytes branch from c696ef9 to bbeeaa2 Compare July 20, 2023 22:14
@cuviper
Copy link
Member

cuviper commented Jul 20, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 20, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4195ddf into rust-num:master Jul 20, 2023
3 checks passed
@cuviper
Copy link
Member

cuviper commented Jul 20, 2023

Sorry again for the very long delays, but this is now published in 0.2.16!

@kaidokert
Copy link
Contributor Author

Thank you !!! Great to see persistence paid off 👍

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.

PrimInt: to_be_bytes & co
6 participants