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

Subject: Re: [boost] [Review] Boost.Atomic review starts today, October 17th 2011
From: andrey.semashev_at_[hidden]
Date: 2011-10-23 14:44:35

Hi,

This is my mini-review of Boost.Atomic. I didn't have much time to evaluate 
the library, but from what I saw this would be a most useful addition to 
Boost. So, my vote is "the library should be accepted to Boost".

Now, to the details. I took a quick reading the docs, and here are my 
comments:
1. Constants BOOST_ATOMIC_CHAR16_LOCK_FREE and BOOST_ATOMIC_CHAR32_LOCK_FREE 
seem to be missing in the docs.
2. There seem to be no "Reference" section that describes the formal 
interfaces and public declarations. The "Programming interfaces" section 
attempts to provide this information but it would be better to have a single 
declaration of boost::atomic template with references to the description of 
its methods (pretty much what Doxygen provides). IMHO, that way the 
information would be more percievable.
3. It would be nice to have examples from the "Usage examples" section in a 
compilable form.

All in all, the documentation seems to be sufficient for a more or less 
experienced programmer (which will probably be the user of the library) to get 
things started. The presence of real-world usage examples also helps a lot.

Design and implementation. I did not dig into the code too much, but I can see 
that the implementation followed the standard quite closely. I mostly 
evaluated x86 part of the implementation.
1. A pity that the functional interface (i.e. atomic_* functions and atomic_* 
types) and the ATOMIC_FLAG_INIT and ATOMIC_VAR_INIT macros are absent. These 
tools might be very useful where atomics need to be statically initialized. 
But even without them the atomic template is very useful.
2. I would prefer the boost/atomic.hpp file to only include some headers in 
boost/atomic directory. Presumably, the boost::atomic template should be in 
the boost/atomic/atomic.hpp, and boost/atomic.hpp should include this file 
(along with other files, if there are any).
3. The library should probably be in the boost::atomics namespace. The atomic 
class template may be imported into boost namespace via using declaration. But 
I don't have a strong case here.
4. About gcc-x86.hpp and PIC. I believe, GCC automatically defines __PIC__ != 
0 when PIC is generated, you could test for it and only save/restore ebx when 
needed.
5. I would really like to have a DCAS-based atomic on the x86-64 platform. 
I've seen the discussion on this topic, just pointing out that even though it 
may be slow on the current processors, things may change in future.
6. I see you use spinlock pool when no hardware support for atomic operations 
is available. Will this work in multi-module applications, especially on 
Windows? I understand that this quesion is probably better to be addressed to 
Boost.SmartPtr maintailer, but still...
7. Tabs should be converted to spaces and every file should end with a 
newline.

Other than that the code looks ok. I did not compile anything though.

Answering the last few questions:

- How much effort did you put into your evaluation? A glance? A quick reading? 
In-depth study?

A quick reading through the docs (about an hour, I guess) and the code (around 
1.5 hours).

- Are you knowledgeable about the problem domain?

I did implement a few lock-free algorithms and I have experience of using 
std::atomic and inline assembly on x86 and x86-64. I wouldn't call myself an 
expert though.

Lastly, I'd like to thank Helge for his huge effort on bringing this library 
to Boost. This is a long overdue addition.