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**.
> To the dev@apr members, anyone feel like calling this 0.9.2 in conjunction
> with httpd's next release and pushing on to 0.9.3-dev???
Okay with me.
Brian
