Add to_bytes to bigint #166
Conversation
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
ff/src/biginteger/macros.rs
Outdated
| #[inline] | ||
| fn to_bits_be(&self) -> Vec<bool> { | ||
| BitIteratorBE::new(self.0).collect::<Vec<_>>() | ||
| } | ||
|
|
||
| #[inline] | ||
| fn to_bits_le(&self) -> Vec<bool> { | ||
| BitIteratorLE::new(self.0).collect::<Vec<_>>() | ||
| } | ||
|
|
||
| #[inline] | ||
| fn to_bytes_be(&self) -> Vec<u8> { | ||
| let mut le_bytes = self.to_bytes_le(); | ||
| le_bytes.reverse(); | ||
| le_bytes | ||
| } |
There was a problem hiding this comment.
I think these can all be made default impls on the trait itself? For the bits, you can call BitIteratorXX::new(self)..., and to_bytes_be is implemented directly in terms of to_bytes_le.
There was a problem hiding this comment.
True, ok I'll do that
There was a problem hiding this comment.
I didn't realize the trait had AsRef<[u64]>, seems kind of word to be in the trait imo. Why would we not allow float back ends?
There was a problem hiding this comment.
Actually I think its weird to only implement one of to_bytes_le and to_bytes_be as the default, I'd prefer it in the macro. (The reason to put it in the default impl would be for users implementing these in an alternate fashion, but that then assumes that to_bytes_le is the native endianness to do it)
There was a problem hiding this comment.
Actually I think we can move to_bytes_le also to the trait, as it can be implemented from AsRef<[u64]>.
(This is an example of why the AsRef<[u64]> trait bound is useful)
There was a problem hiding this comment.
I'll move it for now, but I think its worth revisiting if BigInt needs AsRef<[u64]>, I feel like we shouldn't make PrimeField / elliptic curves depend on that. (Agreed that its fine for the prime field generation macro though)
There was a problem hiding this comment.
We lose capacity hints with the to_bytes being a default implementation, not really sure its worth making it default then?
There was a problem hiding this comment.
Ah no, I was wrong. The capacity hints are still available
Description
Adds a simple
to_bytesmethod tobigint. Currently there isto_bitsandwrite, andwriteis a bit awkward to use if you're not concerned with efficiency of the serialization.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer