Skip to content

Use u16::to_be_bytes#1120

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:07-22-to-be-bytes
Jul 25, 2022
Merged

Use u16::to_be_bytes#1120
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:07-22-to-be-bytes

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jul 22, 2022

Now that MSRV is > 1.32 we can use u16::to_be_bytes to ensure network byte order when encoding the port number of a AddrV2Message.

Remove the TODO and use to_be_bytes as suggested.

@tcharding tcharding changed the title Use to_be_bytes Use u16::to_be_bytes Jul 22, 2022
Kixunil
Kixunil requested changes Jul 22, 2022
View reviewed changes
Comment thread src/network/address.rs Outdated
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Jul 22, 2022

Choose a reason for hiding this comment

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

write_all

Copy link
Copy Markdown
Member Author

@tcharding tcharding Jul 23, 2022

Choose a reason for hiding this comment

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

Gets me every time.

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Jul 24, 2022

Choose a reason for hiding this comment

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

No worries, maybe it'll become non-issue. See #1123 (comment)

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jul 22, 2022

I suspect we have clippy broken. This lint should've fired: https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jul 22, 2022

Oh, it's actually a clippy bug. Please use method call instead. It'll be clearer anyway.

@Kixunil Kixunil mentioned this pull request Jul 22, 2022
Closed
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jul 22, 2022

I think the lint actually wouldn't fire regardless, because we do "use" the result by adding it to len.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jul 23, 2022

@apoelstra oh, yes, damn! Funny it still reproduces.

@tcharding tcharding force-pushed the 07-22-to-be-bytes branch from 7b2a277 to ff75807 Compare July 23, 2022 20:36
Kixunil
Kixunil previously approved these changes Jul 24, 2022
View reviewed changes
Copy link
Copy Markdown
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 ff7580708fa08de4cb46e48558354d3214294aba

But if it can be changed to method before anyone else reviews, it'd be nice.

Comment thread src/network/address.rs Outdated
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Jul 24, 2022

Choose a reason for hiding this comment

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

Any reason to not use the method?

Copy link
Copy Markdown
Member Author

@tcharding tcharding Jul 25, 2022

Choose a reason for hiding this comment

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

Done, thanks.

@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 aggregate release notes mention labels Jul 24, 2022
@tcharding
Use to_be_bytes
3c28694 
Now that MSRV is > 1.32 we can use `u16::to_be_bytes` to ensure network
byte order when encoding the port number of a `AddrV2Message`.

Remove the TODO and use `to_be_bytes` as suggested.
@tcharding tcharding force-pushed the 07-22-to-be-bytes branch from ff75807 to 3c28694 Compare July 25, 2022 03:40
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jul 25, 2022

Changes in force push

  • Used w.write_all as I should have in the first place.

Kixunil
Kixunil approved these changes Jul 25, 2022
View reviewed changes
Copy link
Copy Markdown
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 3c28694

apoelstra
apoelstra approved these changes Jul 25, 2022
View reviewed changes
Copy link
Copy Markdown
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 3c28694

@apoelstra apoelstra merged commit cf8c581 into rust-bitcoin:master Jul 25, 2022
@tcharding tcharding deleted the 07-22-to-be-bytes branch July 26, 2022 00:59
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1120: Use u16::to_be_bytes
bd00fbb 
3c28694 Use to_be_bytes (Tobin C. Harding)

Pull request description:

  Now that MSRV is > 1.32 we can use `u16::to_be_bytes` to ensure network byte order when encoding the port number of a `AddrV2Message`.

  Remove the TODO and use `to_be_bytes` as suggested.

ACKs for top commit:
  Kixunil:
    ACK 3c28694
  apoelstra:
    ACK 3c28694

Tree-SHA512: b5dd9b0f43257f84defb9e9ecf9d02469f1959669367c5ad02cfb43003b780cf07ea344579b52a75c424fd85f8fa02dee3713b967c9b3a33eec6b7aa914763cd
@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

aggregate release notes mention code quality Makes code easier to understand and less likely to lead to problems 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.

3 participants

@tcharding @Kixunil @apoelstra