Re: [boost] [atomic] review results

Subject: Re: [boost] [atomic] review results
From: Helge Bahmann (hcb_at_[hidden])
Date: 2011-11-07 15:23:39

On Monday 07 November 2011 18:06:33 Peter Dimov wrote:
> >    Helge Bahmann's boost.atomic library is ACCEPTED.
>
> Congratulations on the acceptance, and a few belated (sorry) comments:
>
> - Boost.Atomic should not use the smart_ptr spinlock pool; it should
> contain its own under boost/atomic. It could be a straight copy of the
> shared_ptr one. Boost.Atomic is lower level than shared_ptr and should not
> have an upward dependency, because a future shared_ptr may (and will) use
> Boost.Atomic in its spinlock implementation.

yes makes sense -- there was a concern raised by Andrey Semashev that the 
spinlock pool as implemented and used by shared_ptr presently may fail on 
Windows due to the pool being non-unique (not had a chance to test this yet), 
and I have found a way to produce a similar failure using dlopen, atomics 
private to shared libraries and RTLD_LOCAL -- currently I am therefore 
leaning on creating a shared library just for the spinlock pool, but since 
you wrote the initial implementation maybe you could comment as well?

> - For the same reason, atomic_flag should be a proper fundamental component
> and not a wrapper over atomic_bool. atomic_flag is the building block for
> spinlocks; it should always be present and should always be lock-free. (And
> it should have ATOMIC_FLAG_INIT, because the spinlock pool needs it.)

okay agreed

> - The interlocked implementation seems suboptimal - it doesn't use
> InterlockedExchange and InterlockedExchangeAdd. (The spelling of
> atomic_signal_fence under MSVC version something and above is
> _ReadWriteBarrier - see boost/smart_ptr/detail/spinlock_w32.hpp).

yes that's indeed an oversight and it is needed

Best regards
Helge