Skip to content

Mutex and RwLock arc method #74942

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
Closed
wants to merge 1 commit into from
Closed

Mutex and RwLock arc method #74942

wants to merge 1 commit into from

Conversation

MarinPostma
Copy link

This PR adds convenient methods Mutex::arc and RwLock::arc according to #74866

Updates all the test to use the new methods instead of the old Arc::new(..)

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2020
@pickfire
Copy link
Contributor

Is the changes in submodule necessary?

@MarinPostma
Copy link
Author

Is the changes in submodule necessary?

You mean in the test submodule?

@pickfire
Copy link
Contributor

You mean in the test submodule?

No, I mean library/stdarch and src/tools/cargo

@MarinPostma
Copy link
Author

MarinPostma commented Jul 30, 2020 •
edited
Loading

I'm not sure where these come from, I will remove them, thank you for noticing it

@PhilipDaniels
Copy link
Contributor

PhilipDaniels commented Aug 1, 2020 •
edited
Loading

I am not an official reviewer so take this with a pinch of salt (from one who is also annoyed by the wordiness of the very frequent Arc<Mutex<>> pattern), but I would have thought if we were going to do this it would be better to follow the established with_ pattern for specialist constructors, namely, add functions Arc::with_mutex and Arc::with_rwlock.

The way you have it at the moment feels a bit 'inside out'.

@MarinPostma
Copy link
Author

@PhilipDaniels I had the Box::pin method in mind that returns a Pin<Box>. I think that when you want to use a mutex, you go look for Mutex and not for Arc.

@bors
Copy link
Collaborator

bors commented Aug 1, 2020

☔ The latest upstream changes (presumably #75016) made this pull request unmergeable. Please resolve the merge conflicts.

@MarinPostma MarinPostma force-pushed the mutex-arc-method branch 2 times, most recently from eed4ee1 to a994377 Compare August 11, 2020 08:59
@MarinPostma MarinPostma requested a review from jyn514 August 13, 2020 15:44
@bors
Copy link
Collaborator

bors commented Aug 27, 2020

☔ The latest upstream changes (presumably #75947) made this pull request unmergeable. Please resolve the merge conflicts.

@MarinPostma MarinPostma force-pushed the mutex-arc-method branch 3 times, most recently from 9d0868f to f86b495 Compare September 5, 2020 07:53
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 5, 2020
jyn514
jyn514 reviewed Sep 5, 2020
View reviewed changes
Cargo.lock Outdated
@@ -126,9 +126,9 @@ dependencies = [

[[package]]
name = "autocfg"
version = "1.0.0"
version = "1.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you meant to update this file.

library/std/src/lib.rs Outdated
@@ -215,6 +215,7 @@
// std is implemented with unstable features, many of which are internal
// compiler details that will never be stable
// NB: the following list is sorted to minimize merge conflicts.
#![cfg_attr(not(bootstrap), feature(doc_spotlight))]
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated, maybe a rebase conflict?

Copy link
Author

Choose a reason for hiding this comment

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

Yup it is, I was waiting for CI, will fix it as when I get to my computer

@bors
Copy link
Collaborator

bors commented Sep 6, 2020

☔ The latest upstream changes (presumably #76385) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2020
@JohnCSimon
Copy link
Member

Ping from triage:
@MarinPostma can you please address the merge conflicts? Thank you.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2020
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2020
@JohnCSimon
Copy link
Member

@MarinPostma unfortunately tests are failing

@JohnCSimon
Copy link
Member

Pinging again from triage:
@MarinPostma unfortunately tests are failing

@Dylan-DPC-zz
Copy link

@MarinPostma thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pickfire pickfire pickfire left review comments

@jyn514 jyn514 jyn514 left review comments

Assignees

@joshtriplett joshtriplett

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@MarinPostma @rust-highfive @pickfire @PhilipDaniels @bors @JohnCSimon @Dylan-DPC-zz @jyn514 @joshtriplett