8000 Use `u8::try_from` by tcharding · Pull Request #1121 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use u8::try_from #1121

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 24, 2022
Merged

Use u8::try_from #1121

merged 1 commit into from
Jul 24, 2022

Conversation

tcharding
Copy link
Member

Currently we use a cast to get the u8 depth, as suggested by the TODO in the code we can now use TryFrom since we bumped the MSRV.

Use u8::try_from.expect("") since depth (node count) is guarded by TAPROOT_CONTROL_MAX_NODE_COUNT.

@dpc
Copy link
Contributor
dpc commented Jul 22, 2022

u18? (:

@tcharding
Copy link
Member Author

lol, took me a while to see that little blighter :)

@tcharding tcharding changed the title Use u18::try_from Use u8::try_from Jul 22, 2022
@dpc
Copy link
Contributor
dpc commented Jul 22, 2022

BTW. I generally want to forbid as keyword cast in my code, but I can't find the directive for that.

@Kixunil
Copy link
Collaborator
Kixunil commented Jul 22, 2022

@dpc propose a clippy lint if there isn't one?

Kixunil
Kixunil previously approved these changes Jul 22, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK e077605eff98e266026e8e7ace7e693bb6b77545

@apoelstra
Copy link
Member

The commit message still says u18 :)

apoelstra
apoelstra previously approved these changes Jul 22, 2022
View reviewed changes
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.

ACK e077605eff98e266026e8e7ace7e693bb6b77545

@tcharding
Copy link
Member Author

The commit message still says u18 :)

Damn, its hard to get good help huh. I only changed the PR title.

@tcharding
Use u8::try_from
517059e 
Currently we use a cast to get the `u8` depth, as suggested by the TODO
in the code we can now use `TryFrom` since we bumped the MSRV.

Use `u8::try_from.expect("")` since, assuming the code comment is
correct, the depth is guaranteed to be less that 127.
@tcharding tcharding dismissed stale reviews from apoelstra and Kixunil via 517059e July 23, 2022 20:41
@tcharding tcharding force-pushed the 07-22-depth-try-from branch from e077605 to 517059e Compare July 23, 2022 20:41
@tcharding
Copy link
Member Author

Fixed commit log, no other changes in force push.

Kixunil
Kixunil approved these changes Jul 24, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 517059e

@Kixunil Kixunil added code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...) one ack PRs that have one ACK, so one more can progress them no release notes mention labels Jul 24, 2022
apoelstra
apoelstra approved these changes Jul 24, 2022
View reviewed changes
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.

ACK 517059e

@apoelstra apoelstra merged commit 9f0ff6f into rust-bitcoin:master Jul 24, 2022
@dpc
Copy link
Contributor
dpc commented Jul 27, 2022

BTW. I generally want to forbid as keyword cast in my code, but I can't find the directive for that.

#![deny(clippy::as_conversions)]

https://rust-lang.github.io/rust-clippy/master/#as_conversions

From https://www.reddit.com/r/rust/comments/w8nxi9/comment/ihqms23/?utm_source=share&utm_medium=web2x&context=3

@tcharding tcharding deleted the 07-22-depth-try-from branch August 1, 2022 05:12
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1121: Use u8::try_from
70289aa 
517059e Use u8::try_from (Tobin C. Harding)

Pull request description:

  Currently we use a cast to get the `u8` depth, as suggested by the TODO in the code we can now use `TryFrom` since we bumped the MSRV.

  Use `u8::try_from.expect("")` since depth (node count) is guarded by `TAPROOT_CONTROL_MAX_NODE_COUNT`.

ACKs for top commit:
  Kixunil:
    ACK 517059e
  apoelstra:
    ACK 517059e

Tree-SHA512: 98e58617247a05025ec13ad3d00a464cb2351c98c6d871be43b252d3982e291b7187e25780e7fe367f5a3a64fa20f3b328876a8901af4da09e675f50727a26cf
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
code quality Makes code easier to understand and less likely to lead to problems no release notes mention one ack PRs that have one ACK, so one more can progress them trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Milestone
0.29.0
Development

Successfully merging this pull request may close these issues.
4 participants
@tcharding @dpc @Kixunil @apoelstra
0