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.

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

Minor improvements to future::join!'s implementation #91721

Merged
merged 6 commits into from
Dec 11, 2021
Merged

Minor improvements to future::join!'s implementation #91721

merged 6 commits into from
Dec 11, 2021

Conversation

danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Dec 9, 2021 •
edited
Loading

This is a follow-up from #91645, regarding some remarks I made.

Mainly:

  • it hides the recursive munching through a private macro, to avoid leaking such details (a corollary is getting rid of the need to use @ to disambiguate);
  • it uses a match binding, outside the async move block, to better match the semantics from function-like syntax;
  • it pre-pins the future before calling into poll_fn, since poll_fn, alone, cannot guarantee that its capture does not move (to clarify: I believe the previous code was sound, thanks to the outer layer of async. But I find it clearer / more robust to refactorings this way 🙂).
  • it uses @ibraheemdev's very neat .ready()?;
  • it renames Took to Taken for consistency with Done (tiny nit 😄).

TODODone:

  • Add unit tests to enforce the function-like :value semantics are respected.

r? @nrc

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2021
ibraheemdev
ibraheemdev approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

👍

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@danielhenrymantilla
Minor improvements to future::join!'s implementation
e936071 
This is a follow-up from rust-lang#91645, regarding [some remarks I made](https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/join!/near/264293660).

Mainly:
  - it hides the recursive munching through a private `macro`, to avoid leaking such details (a corollary is getting rid of the need to use `@` to disambiguate);
  - it uses a `match` binding, _outside_ the `async move` block, to better match the semantics from function-like syntax;
  - it pre-pins the future before calling into `poll_fn`, since `poll_fn`, alone, cannot guarantee that its capture does not move;
  - it uses `.ready()?` since it's such a neat pattern;
  - it renames `Took` to `Taken` for consistency with `Done`.
@danielhenrymantilla
Fix two false positive lints
846cb9c
@danielhenrymantilla @ibraheemdev
Bring back the colon separators for the macro munching.
07bcf4a 
Co-Authored-By: Ibraheem Ahmed <ibrah1440@gmail.com>
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@danielhenrymantilla @ibraheemdev
Fix missing mut typo
e277a98 
Co-authored-by: Ibraheem Ahmed <ibrah1440@gmail.com>
@danielhenrymantilla
Copy link
Contributor Author

r? @joshtriplett, as discussed over Zulip

@rust-highfive rust-highfive assigned joshtriplett and unassigned nrc Dec 9, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 10, 2021

📌 Commit f8dc13d has been approved by joshtriplett

@bors
Copy link
Contributor

bors commented Dec 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Dec 10, 2021
@joshtriplett @danielhenrymantilla
Update library/core/tests/future.rs
67ab53d 
Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 10, 2021

📌 Commit 67ab53d has been approved by joshtriplett

@bors
Copy link
Contributor

bors commented Dec 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2021
@bors
Auto merge of rust-lang#91799 - matthiaskrgr:rollup-b38xx6i, r=matthi…
928783d 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#83174 (Suggest using a temporary variable to fix borrowck errors)
 - rust-lang#89734 (Point at capture points for non-`'static` reference crossing a `yield` point)
 - rust-lang#90270 (Make `Borrow` and `BorrowMut` impls `const`)
 - rust-lang#90741 (Const `Option::cloned`)
 - rust-lang#91548 (Add spin_loop hint for RISC-V architecture)
 - rust-lang#91721 (Minor improvements to `future::join!`'s implementation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ed81098 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
@camelid
Copy link
Member

camelid commented Dec 12, 2021

Could this have caused the UB error in https://github.com/rust-lang/miri-test-libstd/runs/4495870835?check_suite_focus=true?

@ibraheemdev ibraheemdev mentioned this pull request Dec 12, 2021
Open
4 tasks
@danielhenrymantilla
Copy link
Contributor Author

Zulip discussion about the UB error: https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Cron.20Job.20Failure.202021-12-12

@danielhenrymantilla
Copy link
Contributor Author

As of https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Cron.20Job.20Failure.202021-12-12/near/264707667, I no longer think this PR is to blame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibraheemdev ibraheemdev ibraheemdev approved these changes

Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
9 participants
@danielhenrymantilla @rust-log-analyzer @joshtriplett @bors @camelid @ibraheemdev @nrc @rust-highfive @rustbot