{Trimming this to @apr for this bit of discussion.}
At 10:16 AM 1/8/2003, Brian Pane wrote:
>On Wed, 2003-01-08 at 07:15, William A. Rowe, Jr. wrote:
>
>> Just an observation reviewing the apr/includes/ changes... I don't like the
>> look of this code;
>>
>> +#define apr_atomic_casptr(mem,with,cmp) (void*)atomic_cmpxchg((unsigned
>> long *)(mem),(unsigned long)(cmp),(unsigned long)(with))
>>
>> Very simply, this is a very easily broken macro... it is too easy to
>> silently
>> pass the wrong pointer arg to apr_atomic_casptr and explode.
>>
>> We need some type safety here, and this macro (I believe) destroys
>> all hope of that. It seems this needs to become a function. Macro to
>> function should be a safe change, since code compiled to an earlier
>> apr will continue to use this unsafe but otherwise usable macro.
>
>There's a good reason why the atomic CAS can't be a function.
>Its most common use is in spinlocks, where the probability of
>having to retry the CAS increases with every clock cycle you
>spend in the "if (cas failed) {retry;}" loop. Adding a function
>call around the CAS would double the length of the code path
>between the lookup of the old value and the compare-and-swap
>of the new value, which in turn doubles the probability that
>the CAS will fail and require a retry.
>
>As for type safety, keep in mind that the role of
>apr_atomic_casptr is to atomically swap any pointer to
>an object. If we were to turn it into a function, its first
>argument would have to be a void**.
In that case... what about a trick (I believe) Ben Laurie taught us?
Using a typedef for clarity:
typedef void*(*apr_atomic_casptr_fn_t)(unsigned long* mem, unsigned long cmp,
unsigned long with);
#define apr_atomic_casptr ((apr_atomic_casptr_fn_t)(atomic_cmpxchg))
In this way, the arguments to apr_atomic_casptr will be evaluated in terms
of the apr_atomic_casptr_fn_t declaration.
This presumes the arguments exactly match the atomic_cmpxchg function,
with the exception of twos-compliment signedness.
Make sense?
