Re: [Boost-users] [Review] Lockfree review starts today, July 18th

Subject: Re: [Boost-users] [Review] Lockfree review starts today, July 18th
From: John Bytheway (jbytheway+boost_at_[hidden])
Date: 2011-07-18 17:48:07

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<T *, freelist_t, Alloc> 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<T *, freelist_t, Alloc>::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<T,0>::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