This is not a review, just some thoughts on a quick reading of the
documentation at http://tim.klingt.org/boost_lockfree/. A mixture of
nitpicks and more serious concerns:
- You provide a mp-mc queue, a sp-sc queue, and a mp-mc stack. Are
there any other combinations you know of for which an implementation
could usefully be provided; e.g. could a sp-mc stack be made faster than
the one you've provided?
- Some of your function declarations have explicitly-void argument lists
(e.g. fifo's default constructor). This is correct, but unconventional.
An empty argument list would be less confusing, IMO.
- You describe it as a 'limitation' that "The class T is required to
have a trivial assignment operator.". I think this is better described
as a 'requirement'. Are there any other requirements? I'm guessing it
must be CopyConstructible, or at least MoveConstructible? Are you sure
you don't need a trivial destructor and/or copy constructor in addition
to the trivial assignment?
- fifo::empty is described as non-thread-safe. To what extent? Might
it crash the program or corrupt the container, or is it simply that
other threads can change the content of the container at a whim, and so
the result of calling empty might not persist until the next operation?
- Does the specialization fifo also require that
T is trivially assignable, or even that it is assignable at all? It's
not clear. If it is actually storing pointers is there a risk of memory
leaks if the container is destroyed while non-empty? If it's not
actually storing pointers, then what am I to do if I genuinely want a
fifo of pointers? Generally I find this specialization a bit worrying
since its semantics differ so markedly from the unspecialized equivalent.
- There is no description of fifo::dequeue(T*&).
- You have documented no requirements on ringbuffer's T type. So I
guess it needn't be trivially assignable; but again I guess it must be
CopyConstructible?
- The ringbuffer docs mention several times the type 'T const (&)',
which doesn't make sense; I'm guessing a '[size]' has been cut out at
some point in the documentation generation process.
- You appear to be missing "Returns:" in the docs for
'ringbuffer::enqueue(T const (&) t)' and 'ringbuffer::enqueue(T const *
t, size_t size)' (and similarly the specialization).
- In ringbuffer docs: "Enqueues size objects from the iterator range
[begin, end[ to the ringbuffer."; this range would normally be written
"[begin, end)"
- There is no description of ringbuffer::is_lock_free, nor
ringbuffer::is_lock_free
- In stack docs, s/poping/popping/
- stack's empty is documented differently from the others; is it
different somehow; more safe? more suitable for non-debugging purposes?
- The fifo and stack insertion functions both return false if "if the
freelist is not able to allocate a new fifo node.". Is this only when
using a fixed-sized freelist which has used all it's size, or might this
also happen with a caching freelist when allocation fails (e.g. due to
running out of memory). To put it another way: how are you handling
exceptions thrown by the allocator?
- BOOST_STATIC_ASSERT is classed as a "private member function"; I guess
this is a side-effect of auto-generated docs or something, but it's
rather odd.
- Why does ringbuffer have range enqueue functionality, but not the
other two classes?
- Why have you provided a pointer specialization for fifo but not stack?
- Since these classes have various methods in common, would it be worth
pulling those out and documenting them once centrally (in the way the
standard does).
John Bytheway
