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