Definition of atomic_cas is confusing

[bremoran]

The definition of atomic_cas is confusing and makes it seem like there are race conditions where none exist.

For example, in the pool allocator, the prototype of atomic_cas makes it appear as though there is a race condition: https://github.com/ARMmbed/core-util/blob/0873979593abf4c632e7dda7358d046f919cf75a/source/PoolAllocator.cpp#L41

It looks like prev_free is only ever assigned on line 36, and that if the function fails once, it will fail forever. This is not the case, but it appears to be.

The in-out parameter is at fault for this confusing behavior.

If atomic_cas had a different signature, it could make this behaviour much more explicit:

template <typename T>
bool atomic_cas(T *ptr, T *currentValue, T expectedValue, T desiredValue);

void* PoolAllocator::alloc() {
    uint32_t prev_free = (uint32_t)_free_block;
    if (0 == prev_free)
        return NULL;
    while (true) {
        void **const new_free = (void **)(*((void **)prev_free));
        uint32_t actual_free;
        if (atomic_cas((uint32_t*)&_free_block, &actual_free, prev_free, (uint32_t)new_free)) {
            return (void*)prev_free;
        }
        prev_free = actual_free;
    }
}

This makes code using atomic_cas much more readable.

cc @bogdanm @rgrover @autopulated @0xc0170

[rgrover]

in all uses of atomic_cas, there will be the need to do prev_free = actual_free in the failure case. The current signature of atomic_cas makes this assignment implicit. atomic_cas is a nontrivial API to grok; but I don't think decomposing T *currentValue into currentValue and expectedValue is helping here. Keeping all these differing but similar names organized in the mind is also going to become expensive and confusing for users.

[bremoran]

I agree that there will always need to be prev_free = actual_free but I believe that aids readability. I also believe that there will be an increase in grokability without the [in,out] parameter. One additional similar name simplifies the mental model of the program flow and so makes it less confusing or expensive for users.

[0xc0170]

I don't think decomposing T currentValue into T currentValue and expectedValue is helping here.

Same for me.

For reference C++11, http://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange which as I understand has similar behavior as the current implementation.

"Atomically compares the object representation of this with the object representation of expected, as if by std::memcmp, and if those are bitwise-equal, replaces the former with desired (performs read-modify-write operation). Otherwise, loads the actual value stored in this into expected (performs load operation). Copying is performed as if by std::memcpy."

[ciarmcom]

ARM Internal Ref: IOTSFW-1305

[bremoran]

@0xc0170 I appreciate the comments, but I should point out that the standard library doesn't do everything right. For example, std::function allocates memory when attaching a pointer to a member function.

The question for me is whether it has the right signature for us, not whether it's the same as the standard library.

[0xc0170]

I should point out that the standard library doesn't do everything right

Agree

The question for me is whether it has the right signature for us, not whether it's the same as the standard library.

I expressed my opinion. no harm intentions with provided reference

[bogdanm]

I agree that the proposed version makes the code somewhat more readable, but even Wikipedia defines it like we do currently (https://en.wikipedia.org/wiki/Compare-and-swap). This seems to be quite a common definition, so I propose to leave it as it is (or, alternatively, give it a different name so that people don't get confused).

[bremoran]

Since the consensus is to keep the standard library version, I will close.