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

Implement (part of) ACP 429: add DerefMut to Lazy[Cell/Lock] #129334

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

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Aug 20, 2024

DerefMut is instantly stable, as a trait impl. That means this needs an FCP.

@rustbot label +needs-fcp

rust-lang/libs-team#429

@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
Copy link
Contributor Author

ChayimFriedman2 commented Aug 20, 2024

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
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
@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.

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.

@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.

@ChayimFriedman2
Copy link
Contributor Author

@rustbot ready

I modified this to only add (immediately stable) DerefMut, after the other methods has been added unstably in #130476. This is now ready for FCP.

Although, libs-api may not want to include this until force_mut() stabilizes, or people may resort to this as a workaround (although I expect DerefMut to be used more often anyway, just like Deref vs. force()).

@rustbot rustbot 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 Nov 16, 2024
@ChayimFriedman2 ChayimFriedman2 changed the title Implement ACP 429: add LazyCell/Lock::get[_mut](), DerefMut and force_mut() Implement (part of) ACP 429: add DerefMut to Lazy[Cell/Lock] Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants