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 AtomicT::update & AtomicT::try_update #133829

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

GrigorenkoPV
Copy link
Contributor

@GrigorenkoPV GrigorenkoPV commented Dec 3, 2024

ACP accepted in rust-lang/libs-team#490

@rustbot label +T-libs-api

@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2024

r? @Noratrieb

rustbot has assigned @Noratrieb.
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. labels Dec 3, 2024
@traviscross
Copy link
Contributor

traviscross commented Dec 4, 2024

During the ACP discussions today, nobody loved the name fetch_update_infallible, and there was openness to going with a better name if one could be found.

My thoughts went toward finding a verb-form antonym for the word "try". In discussion with @joshtriplett after the meeting, he insightfully observed that the verb form antonym of "try" is "do or do not" (h/t Yoda).

In all seriousness, though, the antonym of "try" is actually "do". To "do something" implies exclusion of simply "trying" to do it.

So, especially as we wish that fetch_update had been called try_fetch_update, perhaps we might consider naming this one do_fetch_update.

pub fn do_fetch_update<F>(
    &self,
    set_order: Ordering,
    fetch_order: Ordering,
    mut f: F,
) -> u64
where
    F: FnMut(u64) -> u64;

When we write that standard library naming convention guide that we talked about in the meeting today, then, we could say that we choose do_ for exactly these kind of cases where the base name is taken and we need a version that does not try_.

@rustbot labels +I-libs-api-nominated

...to consider the new naming proposal.

cc @rust-lang/libs-api

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 4, 2024
@Noratrieb Noratrieb added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2024
@traviscross
Copy link
Contributor

traviscross commented Dec 17, 2024

In the libs-api call today, (enough) people didn't like do_fetch_update, but it did prompt a productive discussion on the name.

In terms of why fetch_update_infallible isn't the right name, Mara pointed out correctly that often exiting early represents a success case, so tying that concept to "failure" or "fallibility" doesn't make sense.

Another name proposed, by David, was fetch_update_always.

Then there was the proposal by Amanieu to deprecate fetch_update and rename both, that Josh will describe.

@joshtriplett
Copy link
Member

We talked about this in today's @rust-lang/libs-api meeting. We didn't come to a firm conclusion yet, but we are considering the possibility of introducing new names for both functions, such as update and try_update. This would correct the issue that fetch_update should have been try_fetch_update.

@dev-ardi
Copy link
Contributor

What would happen to the old occurrences of fetch_update?
Would they be marked as deprecated?
Is it worth the churn?
Should we add a lint to detect when fetch_update should have been a fetch_update_infallible (or whatever the name will be)?

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 23, 2024
@traviscross traviscross removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 7, 2025
@Amanieu Amanieu 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 14, 2025
@m-ou-se m-ou-se 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 14, 2025
@Amanieu
Copy link
Member

Amanieu commented Jan 14, 2025

Since this was still marked as waiting-on-team, we would like to clarify the team's decision: we would like update and try_update methods to be added and the old fetch_update method to be deprecated in favor of those.

@GrigorenkoPV
Copy link
Contributor Author

GrigorenkoPV commented Jan 14, 2025

Since this was still marked as waiting-on-team, we would like to clarify the team's decision: we would like update and try_update methods to be added and the old fetch_update method to be deprecated in favor of those.

What is the process on this? update & try_update get added as unstable, then, later, when they are stabilized, fetch_update is marked as deprecated? Or do we skip some steps and add something as stable immediately?

I'm hesitant to move forward before clarification, so
@rustbot label -S-waiting-on-author +S-waiting-on-team


On an unrelated note, what happened to the generic Atomic<T>? I thought it had been added as unstable, but I couldn't find it anywhere. Did it get removed or am I misremembering something?

@rustbot rustbot added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2025
@Sky9x
Copy link
Contributor

Sky9x commented Jan 18, 2025

Another naming option that I don't think anyone has brought up yet is fetch_modify. This function is essentially performing a "custom" atomic RMW (read-modify-write) operation.
For example, one could write foo.fetch_modify(|x| x.saturating_add(1)) for saturating atomic addition as opposed to the wrapping behavior of fetch_add.

I personally would prefer to avoid deprecating another1 atomic method, and retaining the fetch_* prefix matches the rest of the RMW operations (eg. fetch_add, fetch_xor, etc).

Footnotes

  1. The first one being CAS (compare_and_swap)

@Amanieu
Copy link
Member

Amanieu commented Jan 21, 2025

What is the process on this? update & try_update get added as unstable, then, later, when they are stabilized, fetch_update is marked as deprecated? Or do we skip some steps and add something as stable immediately?

Add it as unstable first, then stabilize, then later deprecate fetch_update.

On an unrelated note, what happened to the generic Atomic<T>? I thought it had been added as unstable, but I couldn't find it anywhere. Did it get removed or am I misremembering something?

#130543

Another naming option that I don't think anyone has brought up yet is fetch_modify. This function is essentially performing a "custom" atomic RMW (read-modify-write) operation.

That's basically just a synonym for fetch_update. It doesn't give any indication of what the difference between fetch_update and fetch_modify is.

I personally would prefer to avoid deprecating another1 atomic method, and retaining the fetch_* prefix matches the rest of the RMW operations (eg. fetch_add, fetch_xor, etc).

The only 2 options that we are current considering are either adding update and try_update or doing nothing and leaving fetch_update as it is. The decision will have to be made when the time comes to stabilize update and try_update as to which option we want to commit to.

@Amanieu Amanieu removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 22, 2025
@GrigorenkoPV GrigorenkoPV force-pushed the fetch_update_infallible branch from 1dd8818 to ab0c68e Compare January 22, 2025 18:34
@GrigorenkoPV GrigorenkoPV changed the title AtomicT::fetch_update_infallible Implement AtomicT::update & AtomicT::try_update Jan 22, 2025
@GrigorenkoPV
Copy link
Contributor Author

@rustbot label -S-waiting-on-team +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 22, 2025
@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the fetch_update_infallible branch from ab0c68e to 541f7bd Compare January 22, 2025 19:53
@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the fetch_update_infallible branch from 541f7bd to c06ed54 Compare January 22, 2025 20:22
@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 28, 2025

📌 Commit c06ed54 has been approved by Noratrieb

It is now in the queue for this repository.

@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 Jan 28, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 28, 2025
…le, r=Noratrieb

Implement `AtomicT::update` & `AtomicT::try_update`

ACP accepted in rust-lang/libs-team#490

`@rustbot` label +T-libs-api
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 28, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#133151 (Trim extra whitespace in fn ptr suggestion span)
 - rust-lang#133829 (Implement `AtomicT::update` & `AtomicT::try_update`)
 - rust-lang#135367 (Enable `unreachable_pub` lint in `alloc`)
 - rust-lang#135748 (Lower index bounds checking to `PtrMetadata`, this time with the right fake borrow semantics 😸)
 - rust-lang#135805 (Add missing allocator safety in alloc crate)
 - rust-lang#135886 (Document purpose of closure in from_fn.rs more clearly)
 - rust-lang#135961 (Fix 2/4 tests skipped by opt-dist)
 - rust-lang#136012 (Document powf and powi values that are always 1.0)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit da5e22d into rust-lang:master Jan 28, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 28, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 28, 2025
Rollup merge of rust-lang#133829 - GrigorenkoPV:fetch_update_infallible, r=Noratrieb

Implement `AtomicT::update` & `AtomicT::try_update`

ACP accepted in rust-lang/libs-team#490

``@rustbot`` label +T-libs-api
@GrigorenkoPV GrigorenkoPV deleted the fetch_update_infallible branch January 29, 2025 13:15
@GrigorenkoPV GrigorenkoPV restored the fetch_update_infallible branch January 29, 2025 13:15
@GrigorenkoPV GrigorenkoPV deleted the fetch_update_infallible branch January 29, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.