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

Subject: Re: [Boost-users] [Review] Lockfree review starts today, July 18th
From: Tim Blechmann (tim_at_[hidden])
Date: 2011-07-18 19:08:42

hi john,

> - 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?

good questions ... possibly, but i am not aware of such an algorithm ...

> - 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.

this is my personal coding style, if people suggest to remove it, i can do that.

> - 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?

hm, you are right. it both needs to have a trivial destructor and a copy 
constructor. i don't want to provide any move semantic, enqueue/push may fail if 
no nodes can be allocated.

> - 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?

it won't crash, but the answer will may be incorrect.

> - 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.

i actually like matthew's proposal of templating the dequeue/pop argument. that 
would make the specialization obsolete.
currently, destroying a non-empty fifo<T*> would leak memory.

> - 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?

correct, i should document this

> - 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.

hrrr ... you are right ... will need to add this to my list of questions for the 
quickbook/doxygen masters ...

> - stack's empty is documented differently from the others; is it
> different somehow; more safe?  more suitable for non-debugging purposes?

yes it is: stack::empty() does just need to check if there is a top node 
available().

> - 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?

no, but i guess for consistency reasons i should do that!

> - 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.

thanks, will hide it for the doxygen pass.

> - Why does ringbuffer have range enqueue functionality, but not the
> other two classes?

the ringbuffer will be able to enqueue the range in one step, stack and fifo 
cannot do this. but maybe it would make sense to add a range interface, but 
document the behavior.

> - Why have you provided a pointer specialization for fifo but not stack?

:) historical reasons ... i should either provide a specializaton for both or 
for none ...

> - 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).

well, there may be subtle differences because of the implementation. e.g. 
fifo::empty() vs stack::empty() ...

thanks for your comments, i will address the issues in the next days ...

cheers, tim