thread review (was Re: New file uploaded to boost /)

From: williamkempf_at_[hidden]
Date: 2001-09-06 08:01:55

--- In boost_at_y..., flameframe_at_h... wrote:
> 
> Some minor comments about a submission of threadcorrect1.zip
> 
> - class thread_specific_ptr miss 'set' function which is required 
to 
> initialize it.

This is the purpose of reset(), as documented.

> - in thread_specific_ptr.html html code uses 
> malformed 'boost::thread_specific_ptr<int> value;' - should 
> be 'boost::thread_specific_ptr&lt;int&gt; value;'

Fixed in CVS.  Thanks.
 
> - below in the example (also in many other examples) non-existing 
> thread_group::create function is used - should be create_thread 
> instead.

Fixed in CVS.  Thanks.
 
> - documentation does not expose a drawback that on Win32 users of 
the 
> library must build and deliver with application an additional 
> threadmon.dll if they want to make use of thread_specific_ptr.

I'm not sure I consider this a drawback, nor do I think such an 
implementation detail needs documentation.
 
> - implementation of call_once will fail when called function throws

I didn't think about this one.  I'm not sure if we should try 
and "fix" call_once or document that such functions must not throw.
 
> - continious use of 
>    boost::xtime xt;
>    boost::xtime_get(&xt, boost::TIME_UTC);
>    xt.sec += ...;
>   is ugly. Why do not provide duration version of sleep/wait   
> everywhere when appropriate? (ok. not with CV). 

My hope is that boost will have a fully implemented xtime type and a 
class wrapper (ctime for want of a better name right now) that would 
allow the following:

cond.wait(ctime() + duration::from_sec(1));

I debated including duration waits but they are not usable with 
condition and so I chose not to include them at all for consistency.
 
> - atomic type was discussed shortly and excluded from the library. 
> reasons are unclear. there should be at least rationale in docs.

It's too low level and difficult to use properly (you run into 
problems with memory access issues similar to the problem with the 
DCL pattern when using this type for "atomic ref-counting", which is 
the reason most folks want this type).  I'm not sure that documenting 
the rationale for this is appropriate, since most users aren't going 
to expect to find such a type any way (the only MT library I'm aware 
of in general use that includes such a concept is Win32).  I'd 
consider writing a FAQ for this, but if we decide to add such a 
concept later (a distinct possibility) we'd have to remove the FAQ.
 
> - in my opinion the library must be logically splitted into 
> synchronization part and thread part. This will allow users to 
build 
> alternative thread designs like thread::ref without lost of other 
> functionality. 

I don't agree.  The synchronization types currently are in headers 
that do not include <boost/thread/thread.hpp>, so there's enough of a 
logical split to allow such alternative designs today.  Further, I'd 
expect such alternative designs to be built on top of boost::thread 
to begin with, to take advantage of the portability already built in.
 
Bill Kempf