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

Implement ACP 429: add LazyCell/Lock::get[_mut](), DerefMut and force_mut() #129334

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement ACP 429: add LazyCell/Lock::get[_mut](), DerefMut and force_mut() #129334

wants to merge 1 commit into from
+333 −13

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Aug 20, 2024 •
edited
Loading

In the implementation of force_mut(), I choose performance over safety. For LazyLock this isn't really a choice; the code has to be unsafe. But for LazyCell, we can have a full-safe implementation, but it will be a bit less performant, so I went with the unsafe approach.

DerefMut is instantly stable, as a trait impl; as such, I decided to also instantly stabilize force_mut(), since it makes little sense to have one but not the other. That means this needs an FCP.

@rustbot label +needs-fcp

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2024

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Aug 20, 2024
@ChayimFriedman2 ChayimFriedman2 mentioned this pull request Aug 20, 2024
Open
5 tasks
@ChayimFriedman2
Copy link
Contributor Author

ChayimFriedman2 commented Aug 20, 2024 •
edited by joboet
Loading

r​? libs-api

@joboet
Copy link
Member

joboet commented Aug 21, 2024

Weird, rustbot seems to be sleeping...
r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 21, 2024
@rustbot rustbot assigned BurntSushi and unassigned joboet Aug 21, 2024
joboet
joboet approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me, r=me with the nit resolved after the FCP completes.

library/std/src/sys/sync/once/queue.rs Outdated Show resolved Hide resolved
@ChayimFriedman2
Implement ACP 429: add LazyCell/Lock::get[_mut](), DerefMut and `…
37732f5 
…force_mut()`

In the implementation of `force_mut()`, I choose performance over safety. For `LazyLock` this isn't really a choice; the code has to be unsafe. But for `LazyCell`, we can have a full-safe implementation, but it will be a bit less performant, so I went with the unsafe approach.

`DerefMut` is instantly stable, as a trait impl; as such, I decided to also instantly stabilize `force_mut()`, since it makes little sense to have one but not the other.
@SteveLauC
Copy link
Contributor

Gentle ping on this PR, would love this to be merged soon:)

@ChayimFriedman2
Copy link
Contributor Author

@SteveLauC This depends on a T-libs-api member starting a FCP.

BurntSushi
BurntSushi requested changes Sep 14, 2024
View reviewed changes
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

New APIs need to land as unstable. And an FCP isn't needed for that. There should also be a tracking issue. And the ACP should be linked.

@rustbot rustbot 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 14, 2024
@scottmcm
Copy link
Member

Looks like this PR includes DerefMut, which is a trait impl, so that part does need an FCP.

Can you split this into two? One that's just T-libs and only adds unstable methods, and one that's T-libs-api with the stable trait impl. The former can land much more easily than the latter.

@ChayimFriedman2
Copy link
Contributor Author

@BurntSushi @scottmcm This PR needs FCP for DerefMut and force_mut(), which, as I said in the OP, I decided to make instantly stable because it makes little sense to separate it from DerefMut.

For get_mut(), I already opened a tracking issue: #129333. I can split the PR though.

@workingjubilee
Copy link
Member

DerefMut is instantly stable, as a trait impl; as such, I decided to also instantly stabilize force_mut(), since it makes little sense to have one but not the other. That means this needs an FCP.

so you're saying we could land force_mut() on nightly as unstable and then decide to stabilize DerefMut with it?

@ChayimFriedman2
Copy link
Contributor Author

DerefMut is instantly stable, as a trait impl; as such, I decided to also instantly stabilize force_mut(), since it makes little sense to have one but not the other. That means this needs an FCP.

so you're saying we could land force_mut() on nightly as unstable and then decide to stabilize DerefMut with it?

That's an option, but that's not what I'm doing in this PR. Instead, I instantly stabilize both.

@workingjubilee workingjubilee mentioned this pull request Sep 17, 2024
Merged
@bors
Copy link
Contributor

bors commented Sep 19, 2024

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

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

@BurntSushi BurntSushi BurntSushi requested changes

@joboet joboet joboet approved these changes

Assignees

@BurntSushi BurntSushi

Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. 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.
8 participants
@ChayimFriedman2 @rustbot @joboet @SteveLauC @scottmcm @workingjubilee @bors @BurntSushi