Rename AtomicPtr::fetch_ptr_{add,sub} to AtomicPtr::fetch_{add,sub}

[WaffleLapkin]

Proposal

Problem statement

AtomicPtr has unstable methods that allow mutating it like other atomic types:

impl<T> AtomicPtr<T> {
    pub fn fetch_ptr_add(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_ptr_sub(&self, val: usize, order: Ordering) -> *mut T;

    pub fn fetch_byte_add(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_byte_sub(&self, val: usize, order: Ordering) -> *mut T;

    pub fn fetch_or(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_and(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_xor(&self, val: usize, order: Ordering) -> *mut T;
}

These methods were added in order to remove some int2ptr casts (as part of the strict provenance initiative) that were forced because AtomicPtr did not have these APIs and AtomicUsize was used instead.

fetch_ptr_add and fetch_ptr_sub names are confusing and inconsistent:

• ptr implies that the second argument is a pointer (like in case of sub_ptr for example), but it isn't
• pointer::{add,sub} don't have the ptr part
• the ptr was meant to suggest that these methods work in units of T (as opposed to fetch_byte_add), but it is already the default for pointers

Motivation, use-cases

The motivation is to make the naming less confusing, more consistent and easier to understand.

The search for fetch_(add|sub)\(.+\) as \* in public repositories doesn't yield many results, so I'm assuming using AtomicUsize as AtomicPtr with fetch_{add,sub} is not a common practice and we don't have almost any data of the use cases.

I do not know if this is because using AtomicUsize as AtomicPtr is not nice or if this is because fetch_add for pointers is not that useful.

Solution sketches

I propose just dropping _ptr part, renaming the methods to fetch_add and fetch_sub.

An alternative would be to come out with a better naming, however it would still be inconsistent with pointer methods (and I couldn't come out with anything which makes sense).

Additionally we should provide good documentation that clearly states (with examples) how these methods work.

Links and related work

• Tracking issue: Tracking Issue for arithmetic and certain bitwise ops on AtomicPtr rust#99108
• Implementation PR: Allow arithmetic and certain bitwise ops on AtomicPtr rust#96935
  • Comment that suggested fetch_add is ambiguous/footgunny and eventually lead to fetch_ptr_add name Allow arithmetic and certain bitwise ops on AtomicPtr rust#96935 (comment)
• Original ACP: Arithmetic and bitwise ops on AtomicPtr #60
• PR that does the proposed rename: Remove ptr from AtomicPtr::fetch_{add,sub} method names rust#102970

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[m-ou-se]

I personally don't think we should value consistency with the non-atomic pointer types above the potential confusion here. I think it's not unusual for code to be upgraded/changed from AtomicUsize to AtomicPtr (especially because of the strict provenance work), and it'd be a shame if .fetch_add() would result in a different operation. I think it'd be better if the user would be forced to pick between pointer addition and bitwise/byte addition. (Naming is hard though. Maybe there's better names than the current method names.)

[Amanieu]

I'll disagree with @m-ou-se here: I think this change is good since it also makes fetch_add consistent with the same function in C++ which also adds in multiples of the element size. This is also consistent with the pointer API which has both add and byte_add methods.

[scottmcm]

That note about sub_ptr is particularly powerful to me.

I think fetch_add or fetch_wrapping_add make sense to me. I guess the wrapping is unnecessary because with atomics there's no way you could possibly know that you'll be in-bounds of something? I don't really know how people make use of atomic pointers in practice to be sure the result is usable for dereferencing -- it makes me wonder if realistically it tends to need to be fetch_update instead, with the normal pointer operations done on the resulting raw pointer.

[thomcc]

I don't really know how people make use of atomic pointers in practice to be sure the result is usable for dereferencing -- it makes me wonder if realistically it tends to need to be fetch_update instead, with the normal pointer operations done on the resulting raw pointer.

It's tricky and algorithm-specific but absolutely possible, and fetch_update would be a substantial pessimization in many cases. The reason it's wrapping_add vs add is more because we wouldn't gain anything from the unsafety.

[joshtriplett]

We discussed this further in a libs meetup. We felt like the same arguments still apply for why this would be potentially error-prone, most notably that conversion from AtomicUsize to AtomicPtr would type-check and compile but change meaning. So, we don't want to use the names fetch_add or fetch_sub. (Thus, closing this change proposal.)

We weren't entirely thrilled with the names fetch_ptr_add or fetch_ptr_sub, and felt like there might be a clearer name, but we intentionally stopped when we started to bikeshed that. :)

We'd be willing to review a change proposal for a better name that's unambiguous and still makes it clear that it operates in type-sized units rather than bytes.