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.