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-19 17:06:43

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

i've already tried to improve the docs (in a separate branch of my git 
repository)

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

well, if i remove the pointer specialization and modify pop/dequeue to use a 
templated argument, there would be a simple requirement that the value type T 
must be convertible to the dequeue type U ():

template <typename T>
fifo {
    template <typename U>
    bool dequeue (U &);
};

for me the main reason for adding a specialization for pointers was to be able 
to dequeue to smart pointers, which could be achieved with this approach.

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

yes, after thinking about it for some time, i actually decided the same way and 
modified my code accordingly!

cheers, tim