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

Refactor windows sockets impl methods #85792

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

mjptree
Copy link
Contributor

@mjptree mjptree commented May 28, 2021

No behavioural changes, but a bit tidier visual flow.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2021
@joshtriplett
Copy link
Member

I'm not an expert on the Windows sockets API. @rustbot ping windows

@rustbot rustbot added the O-windows Operating system: Windows label May 28, 2021
@rustbot
Copy link
Collaborator

rustbot commented May 28, 2021

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021
@JohnTitor
Copy link
Member

Indeed this shouldn't have any semantic changes and I agree there're some improvements e.g. using map(drop), extracting some code that is unnecessary to be in match. But on the other hand, I'm not sure if some other changes are improvements, e.g. renaming (I think it's obvious that what len or fam means in the short scope, for instance), binding, shuffling match arms. Could you elaborate?

@mjptree
Copy link
Contributor Author

mjptree commented Jun 14, 2021

@JohnTitor The renamed variables were a suggestions to use full names instead of abbreviations, but I agree they may not be improvements. The shuffling of match arms had multilple motivations: a) avoid calling GetLastError multiple times to retrieve the same error code (e.g. Socket::recv_with_flags), b) unnest nested matches (e.g. Socket::new), and c) clarify cases in which match cause divergent control flow (e.g. Socket::connect_timeout). The binding was in almost all cases aimed at reducing the scope of unsafe blocks to the invocation of the C function alone. I found it particularly helpful to recognize, that some blocks encompassed multiple unsafe invocations in single block alongside "safe" code.

@JohnTitor
Copy link
Member

That makes sense, thanks for clarifying! Then I think it's good to go.

@JohnTitor
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 14, 2021

📌 Commit 78d3d37 has been approved by JohnTitor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 14, 2021
…=JohnTitor

Refactor windows sockets impl methods

No behavioural changes, but a bit tidier visual flow.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#80269 (Explain non-dropped sender recv in docs)
 - rust-lang#82179 (Add functions `Duration::try_from_secs_{f32, f64}`)
 - rust-lang#85608 (Stabilize `ops::ControlFlow` (just the type))
 - rust-lang#85792 (Refactor windows sockets impl methods)
 - rust-lang#86220 (Improve maybe_uninit_extra docs)
 - rust-lang#86277 (Remove must_use from ALLOWED_ATTRIBUTES)
 - rust-lang#86285 (:arrow_up: rust-analyzer)
 - rust-lang#86294 (Stabilize {std, core}::prelude::rust_*.)
 - rust-lang#86306 (Add mailmap entries for myself)
 - rust-lang#86314 (Remove trailing triple backticks in `mut_keyword` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3f4d6d7 into rust-lang:master Jun 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 15, 2021
@mjptree mjptree deleted the refactor-windows-sockets branch November 19, 2021 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants