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

On 19/07/11 00:08, Tim Blechmann wrote:

...

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

Well, indeed it seemed unlikely that move semantics could usefully be supported.

...

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

The docs should clarify exactly what can go wrong.

...

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

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

I don't like this. Fundamentally, you must decide whether a container of pointers is responsible for the memory management of the pointers in it. Compare, for example, std::vector and boost::ptr_vector<T>. Your interface is much more akin to the std:: containers, and I think it's wiser to stick to that model. If your users store pointers in it they should concern themselves with the corresponding memory management elsewhere. In particular, the enqueue function should take a T*, not a T as it presently does. Adding half-hearted support to help them will only lead to confusion, obscure memory leaks and problems in the long run. If you want to have a container that does manage the memory of the pointers in it, then you should create an entirely separate class for that, rethink the interface from scratch, and take care of all the corner cases like strong exception safety and freeing memory on destruction. This would certainly be a nice thing to have, but I don't suggest you try to squeeze it into your first version of Boost.Lockfree at this point.

...

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

It's not clear that you want to catch the exceptions. Personally I would expect you not to, but the docs should certainly clarify either way. And you should check what guarantees your methods offer in the face of such exceptions, and document them (I imagine the strong guarantee should be no trouble here). John