On Monday 24 October 2011 12:41:53 Anthony Williams wrote:
On 17/10/11 00:45, Tim Blechmann wrote:
The review of Helge Bahmann's Boost.Atomic library starts today, October 17th 2011. It will end on October 26th.
Just a few quick comments about correctness.
* Operations build on top of compare_exchange (such as fetch_xor in gcc-x86.hpp) use memory_order_relaxed for all operations. Are you sure there are enough compiler memory barriers? Shouldn't you pass the supplied memory order through to the CAS call?
only the "failure order" of CAS is specified as memory_order_relaxed, while the specified order is passed through as "success order" -- since it is retried until it succeeds, it should be okay
* In gcc-ppc.h the BOOST_ATOMIC_ASM_SLOWPATH_CLEAR macro includes a 1: label, yet many of the asm blocks that use it also include a 1: label. Is this correct?
"accidentally" yes, as the "f" and "b" specifiers distinguish between the different "1:" labels :) -- it is however very confusing with no benefit (and I certainly did not intend writing it that way), therefore changing it
* In gcc-x86.h, the platform_cmpxchg64_strong implementation assumes that T is an integral type, such that you can do (int)(desired>>32) to take the top 32 bits, and (int)desired to take the low 32 bits, which is essentially just the signed and unsigned variants of the 64-bit integer. This should therefore not be a template, but a plain inline function. cas64strong.hpp uses it incorrectly for pointers, but this specialization is unnecessary anyway --- this file is only relevant for 32-bit x86.
it specializes to signed/unsigned, but you are right that could be resolved by a cast instead of a template (in general I want to preserve type-safety such as "long != long long" even if both are 64 bit; maybe this is not worth it) Best regards Helge