Re: [boost] [Boost-users] [Review] Boost.Atomic review starts today, October 17th 2011

Subject: Re: [boost] [Boost-users] [Review] Boost.Atomic review starts today, October 17th 2011
From: Helge Bahmann (hcb_at_[hidden])
Date: 2011-10-24 07:16:50

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