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

Big integers (Uint*) from byte slice array with from_be_bytes #436

Merged
merged 2 commits into from Sep 9, 2020
Merged

Big integers (Uint*) from byte slice array with from_be_bytes #436

merged 2 commits into from Sep 9, 2020

Conversation

dr-orlovsky
Copy link
Collaborator

No description provided.

@elichai
Copy link
Member

elichai commented Jun 21, 2020

Could you please implement this without heap allocations?
also, you can add a slice_to_u64_be here and use that:

define_slice_to_be!(slice_to_u32_be, u32);

Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it reads correctly, but I'd be happier with a unit test.

@dr-orlovsky
Copy link
Collaborator Author

also, you can add a slice_to_u64_be here and use that:

@elichai I did, but it fails to compile since this function is not being used from anywhere (i.e. triggers dead_code error):

error: function is never used: `slice_to_u64_be`
  --> src/util/endian.rs:4:9
   |
4  |         pub fn $name(slice: &[u8]) -> $type {
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
55 | define_slice_to_be!(slice_to_u64_be, u64);
   | ------------------------------------------ in this macro invocation
   |
note: the lint level is defined here
  --> src/lib.rs:43:9
   |
43 | #![deny(dead_code)]
   |         ^^^^^^^^^
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

@elichai
Copy link
Member

elichai commented Jul 21, 2020

I meant that you'll use that instead of: u64::from_be_bytes(...) (for MSRV reasons)

@dr-orlovsky
Copy link
Collaborator Author

Oh yes, true

@dr-orlovsky
Copy link
Collaborator Author

done

stevenroose
stevenroose previously approved these changes Aug 9, 2020
@dr-orlovsky
Copy link
Collaborator Author

Now, with #435 merged, it will be nice to merge this one as well before the next version release

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think it could be more readable/idiomatic if it used iterators.

src/util/uint.rs Outdated Show resolved Hide resolved
Co-authored-by: Sebastian <geisler.sebastian@googlemail.com>
@dr-orlovsky
Copy link
Collaborator Author

dr-orlovsky commented Sep 3, 2020

Btw, trying to understand how util::endianis different from https://github.com/rust-bitcoin/bitcoin_hashes/blob/master/src/util.rs#L149-L210 and why not to move there all these functions (old and new ones)? It looks like just a copy-past from bitcoin_hashes::util

Updt: created an issue for that #467 This can be fixed independently from this PR

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@apoelstra apoelstra merged commit c8633b5 into rust-bitcoin:master Sep 9, 2020
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.

None yet

6 participants