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

Added Arc::try_pin #85579

Merged
merged 1 commit into from
Jul 15, 2021
Merged

Added Arc::try_pin #85579

merged 1 commit into from
Jul 15, 2021

Conversation

alex
Copy link
Member

@alex alex commented May 22, 2021

This helper is in line with other other allocation helpers on Arc.

I didn't think this would require an RFC or broader discussion, let me know if that's incorrect.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 22, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@SkiFire13
Copy link
Contributor

Shouldn't you also add Box::try_pin and Rc::try_pin for consistency? What about try_pin_in?

@alex
Copy link
Member Author

alex commented May 22, 2021

I'm not sure, should I? :-) This was the one I had the immediate need for, so I started here.

@ojeda ojeda mentioned this pull request May 25, 2021
Closed
@alex alex mentioned this pull request May 25, 2021
Merged
@yaahc
Copy link
Member

yaahc commented Jun 3, 2021

Shouldn't you also add Box::try_pin and Rc::try_pin for consistency? What about try_pin_in?

IMO, given that both Box and Rc have pin and try_new I feel like adding try_pin to all of them would be appropriate. Similarly, pin_in is only available on Box, so I'd prefer to leave that out until someone asks for it.

Curious to see what the rest of @rust-lang/libs thinks though.

@crlf0710 crlf0710 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 26, 2021
@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 Jul 12, 2021
@Dylan-DPC-zz
Copy link

@yaahc any updates on this?

@ojeda ojeda mentioned this pull request Jul 14, 2021
Open
7 tasks
@yaahc
Copy link
Member

yaahc commented Jul 14, 2021

Not on my end. I was waiting for a reply indicating they planned on adding the additional try_pin functions for Box and Rc or one indicating they'd prefer I just merge it as is. I'm fine either way.

@alex
Copy link
Member Author

alex commented Jul 14, 2021

I thought I sent a reply... but obviously I didn't.

As I said, our use case is for Arc::try_pin only, so left to my own devices it's all I'd implement. However if the libs team would prefer it on Rc and Box for symmetry in order to get this landed, I'm happy to do that.

@yaahc
Copy link
Member

yaahc commented Jul 14, 2021 •
edited
Loading

Lets just merge it as is and I'll bring up adding the other APIs in the next libs team meeting

@bors r+

@bors
Copy link
Contributor

bors commented Jul 14, 2021

📌 Commit 1eb439a has been approved by yaahc

@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 Jul 14, 2021
@alex
Copy link
Member Author

alex commented Jul 14, 2021 via email

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 15, 2021
@JohnTitor
Rollup merge of rust-lang#85579 - alex:patch-1, r=yaahc
fc14c48 
Added Arc::try_pin

This helper is in line with other other allocation helpers on Arc.

I didn't think this would require an RFC or broader discussion, let me know if that's incorrect.
@JohnTitor JohnTitor mentioned this pull request Jul 15, 2021
Closed
@JohnTitor
Copy link
Member

Failed in rollup: #87148 (comment)
@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2021
@alex
Added Arc::try_pin
a214911 
This helper is in line with other other allocation helpers on Arc.
@alex
Copy link
Member Author

alex commented Jul 15, 2021

Fixed.

The issue was that on master the use Pin was behind a cfg.

@JohnTitor
Copy link
Member

Thanks!
@bors r=yaahc

@bors
Copy link
Contributor

bors commented Jul 15, 2021

📌 Commit a214911 has been approved by yaahc

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2021
@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Testing commit a214911 with merge 11c807461082ba506ee42ac8a92cb8e95b9054f2...

@JohnTitor JohnTitor mentioned this pull request Jul 15, 2021
Merged
@JohnTitor
Copy link
Member

@bors retry rolledup

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Testing commit a214911 with merge 5062f7b0641234440f969a3a8db3fa0d7df87f46...

@JohnTitor
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Testing commit a214911 with merge 38268b7f72327b9df43d8c9330aca37550d4fdd5...

@JohnTitor
Copy link
Member

@bors retry rolled-up

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Testing commit a214911 with merge 395c6e176c95d74ffeb104b2f42901586e7259a1...

@JohnTitor
Copy link
Member

Sorry for the noise, CI doesn't love me today...
@bors retry

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2021
@bors
Auto merge of rust-lang#87156 - JohnTitor:rollup-osuhe53, r=JohnTitor
0a6c636 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#85579 (Added Arc::try_pin)
 - rust-lang#86478 (Add -Zfuture-incompat-test to assist with testing future-incompat reports.)
 - rust-lang#86947 (Move assert_matches to an inner module)
 - rust-lang#87081 (Add tracking issue number to `wasi_ext`)
 - rust-lang#87127 (Add safety comments in private core::slice::rotate::ptr_rotate function)
 - rust-lang#87134 (Make SelfInTyParamDefault wording not be specific to type defaults)
 - rust-lang#87147 (Update cargo)
 - rust-lang#87154 (Fix misuse of rev attribute on <a> tag)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors bors merged commit 10f335f into rust-lang:master Jul 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 15, 2021
@alex alex deleted the patch-1 branch July 15, 2021 17:17
@yaahc
Copy link
Member

yaahc commented Jul 23, 2021

Lets just merge it as is and I'll bring up adding the other APIs in the next libs team meeting

So we discussed this in our last libs-api meeting on wednesday and the general consensus was that these methods are niche enough where it is likely unimportant for us to add them to Box and Rc so we're good for now. Thank you again.

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

@yaahc yaahc

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.55.0
Development

Successfully merging this pull request may close these issues.