Skip to content

Replace the generic parse_int_be with a macro called twice #2934

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

parse_int_be is generic across integer types and also input
types, but to do so it relies on the num-traits crate. There's
not a lot of reason for this now that std has from_be_bytes, so
we drop the generic now and replace it with a macro which is called
twice to create two functions, both only supporting conversion from
u5 arrays.

`lightning-invoice` was mostly written before std's `from_be_bytes`
was stabilized, so used its own `to_int_be` utility to do int
conversions from `u8` arrays. Now that the std option has been
stable for quite some time, we should juse use it instead.
`parse_int_be` is generic across integer types and also input
types, but to do so it relies on the `num-traits` crate. There's
not a lot of reason for this now that std has `from_be_bytes`, so
we drop the generic now and replace it with a macro which is called
twice to create two functions, both only supporting conversion from
`u5` arrays.
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.19%. Comparing base (f5ee8c2) to head (39c1d6b).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2934      +/-   ##
==========================================
- Coverage   89.20%   89.19%   -0.01%     
==========================================
  Files         117      117              
  Lines       95513    95513              
  Branches    95513    95513              
==========================================
- Hits        85203    85197       -6     
- Misses       7822     7833      +11     
+ Partials     2488     2483       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Generally LGTM, one question

@valentinewallace
Copy link
Contributor

valentinewallace commented Mar 21, 2024

Should we go ahead and remove the macro and parse_u64_be and parse_u16_be?

Edit: nvm, read the PR description more carefully.

@valentinewallace valentinewallace merged commit 19bcb1c into lightningdevkit:main Mar 21, 2024
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.

4 participants