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

Refactor serve_tcp code #1042

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 3 commits into from
Jun 16, 2022
Merged

Refactor serve_tcp code #1042

merged 3 commits into from
Jun 16, 2022

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented Jun 7, 2022 •
edited
Loading

Done while clearing clippy warnings, done as a separate PR because its not a simple glance to review like the others.

Remove 2 clippy warnings and remove unnecessary local variable.

@tcharding tcharding force-pushed the 06-07-refactor-serve-tcp branch from 7ccfa7f to 03ccc6d Compare June 7, 2022 04:54
tcharding added 3 commits June 7, 2022 14:55
@tcharding
Use write_all instead of write
380e001 
In this unit test we want to write all the pieces, use `write_all`.

Clears clippy warning about not using return value of `write`.
@tcharding
Remove unused loop
04b09a4 
We only simulate a single connection in the test function `serve_tcp`.
Remove the unused loop (includes an unconditional break after first
iteration) and use `next` directly.

Found by clippy. Refactor only, no logic changes.
@tcharding
Remove unnecessary local variable
abfeb32 
In test functions; we bind to `istream` only to re-bind immediately to
`stream`, this is unnecessary and adds no additional information to the
code.
@tcharding tcharding force-pushed the 06-07-refactor-serve-tcp branch from 03ccc6d to abfeb32 Compare June 7, 2022 04:56
@tcharding tcharding mentioned this pull request Jun 7, 2022
Merged
apoelstra added a commit that referenced this pull request Jun 8, 2022
@apoelstra
Merge #1043: Clear clippy warnings from --all-targets
8fd7008 
271d0ba Allow many arguments in test function (Tobin C. Harding)
c0c88fe Use vec instead of pushing to a mutable vector (Tobin C. Harding)
73066e7 Use values() to iterate map values (Tobin C. Harding)
38ff025 Remove useless use of vec! (Tobin C. Harding)
d8e82d5 Remove length comparison to zero (Tobin C. Harding)
c1f34f5 Return Address directly (Tobin C. Harding)
ff8d585 Use flat_map instead of map().flatten() (Tobin C. Harding)
b24a112 Remove calls to clone from types that implement Copy (Tobin C. Harding)
2b8d93e Remove unnecessary explicit reference (Tobin C. Harding)
ef90e3d Use plus-equals operator (Tobin C. Harding)
922b820 Replace assert!(false) with panic! (Tobin C. Harding)
a8039e1 Remove redundant clone (Tobin C. Harding)
cf8de73 Remove unnecessary cast of integer literal (Tobin C. Harding)
999ac45 Do not use assert_eq with literal bool (Tobin C. Harding)
827fcd8 Allow unusual digit grouping (Tobin C. Harding)
242c640 Remove redundant field names (Tobin C. Harding)
0f8f4c5 Collapse if statements (Tobin C. Harding)
229fcb9 Use if let instead of destructuring pattern (Tobin C. Harding)

Pull request description:

  Clear all the clippy warnings (excl. #1042) that are returned by running `cargo clippy --all-targets`.

  I apologize in advance for the review burden :)

ACKs for top commit:
  elichai:
    ACK 271d0ba
  apoelstra:
    ACK 271d0ba

Tree-SHA512: 71ad2ec3db808e795791b7513f8b2a1c13dc90317f5328602c9ecbc31c09f82471f79c9c31a71a0ded5280554d1019d2bb4899fb9e8fa6421d46a2397cd31242
apoelstra
apoelstra approved these changes Jun 8, 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 abfeb32

Kixunil
Kixunil approved these changes Jun 15, 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 abfeb32

Don't want to block a clear improvement but accept would be cool. :)

src/network/stream_reader.rs
ostream.shutdown(Shutdown::Both).unwrap();
break;
// We only simulate a single connection.
let mut ostream = listener.incoming().next().unwrap().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think let mut (ostream, _) = listener.accept().unwrap(); would be simpler and easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is better. Since we have multiple acks already I'll do it as a follow up PR. Cheers.

src/network/stream_reader.rs
// We only simulate a single connection.
let mut ostream = listener.incoming().next().unwrap().unwrap();
for piece in pieces {
ostream.write_all(&piece[..]).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice job spotting write_all!

Copy link
Member Author

Choose a reason for hiding this comment

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

Clippy did that :)

@apoelstra apoelstra merged commit e00b0c3 into rust-bitcoin:master Jun 16, 2022
@tcharding tcharding deleted the 06-07-refactor-serve-tcp branch July 20, 2022 01:39
tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Jul 22, 2022
@tcharding
Use listener.accept()
b1faf63 
During test network simulation we only accept a single connection, we
can simplify the code by using `accept`.

Done as a follow up to review suggestion:

rust-bitcoin#1042 (comment)
@tcharding tcharding mentioned this pull request Jul 22, 2022
Merged
apoelstra added a commit that referenced this pull request Jul 22, 2022
@apoelstra
Merge #1119: Use listener.accept()
0469838 
b1faf63 Use listener.accept() (Tobin C. Harding)

Pull request description:

  During test network simulation we only accept a single connection, we can simplify the code by using `accept`.

  Done as a follow up to review suggestion:

  #1042 (comment)

ACKs for top commit:
  Kixunil:
    ACK b1faf63
  apoelstra:
    ACK b1faf63

Tree-SHA512: b2ead15d3108db3e01d9faab5e3521403dad6a0f4c3cf505f88fefd020110c520a89b9406484c10b04c9a34073c8abc465941577b17b5a193b54502c23b14c61
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1042: Refactor serve_tcp code
8543004 
abfeb32 Remove unnecessary local variable (Tobin C. Harding)
04b09a4 Remove unused loop (Tobin C. Harding)
380e001 Use write_all instead of write (Tobin C. Harding)

Pull request description:

  Done while clearing clippy warnings, done as a separate PR because its not a simple glance to review like the others.

  Remove 2 clippy warnings and remove unnecessary local variable.

ACKs for top commit:
  apoelstra:
    ACK abfeb32
  Kixunil:
    ACK abfeb32

Tree-SHA512: 965708999c067dd8c156bbc54b711f608d524fab7051a0e56066f53b5c8d7bea1c233f04e77873b2624cd22e26a58f1d22f47870d2afe4347aa85335c3142245
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@tcharding
Use listener.accept()
22147a5 
During test network simulation we only accept a single connection, we
can simplify the code by using `accept`.

Done as a follow up to review suggestion:

rust-bitcoin/rust-bitcoin#1042 (comment)
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1119: Use listener.accept()
6a45ebf 
b1faf63 Use listener.accept() (Tobin C. Harding)

Pull request description:

  During test network simulation we only accept a single connection, we can simplify the code by using `accept`.

  Done as a follow up to review suggestion:

  rust-bitcoin/rust-bitcoin#1042 (comment)

ACKs for top commit:
  Kixunil:
    ACK b1faf63
  apoelstra:
    ACK b1faf63

Tree-SHA512: b2ead15d3108db3e01d9faab5e3521403dad6a0f4c3cf505f88fefd020110c520a89b9406484c10b04c9a34073c8abc465941577b17b5a193b54502c23b14c61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpc dpc dpc approved these changes

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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