Re: Formal Review: Signals

From: bill_kempf (williamkempf_at_[hidden])
Date: 2002-02-19 18:19:30

Note that this is my personal review of the Signals library and not a 
stance as the review manager.

First, I vote to accept the library as it provides needed 
functionality and does so with a design that I like.

Now for actual comments.

Documentation:

* My experience with the mutex types in Boost.Threads suggests that 
Signals needs a FAQ and one of the entries needed is a description of 
how the noncopyable behavior of boost::signal does not have to mean 
that objects containing a boost::signal need to be noncopyable as 
well.

* I think that maybe the tutorial should show an actual example of 
using a "named slot" that uses something other than std::string for 
the "name".

* I'd like to see discussion of the visit_each() template in the 
Boost.Signals Design page.  Each a very powerful and useful little 
template and hasn't been given enough visibility in the 
documentation.  It's a useful concept for things other then the 
Boost.Signals library implementation.  (I personal request would be 
for you to make the reasons for the use of the third parameter more 
clear.)  BTW, the documentation for visit_each() states the third 
parameter type to be int, while the description mentions long and the 
actual source uses long.

Tests:

* The use of other Boost libraries in the test programs is at least a 
little suspect.  Failures in these other libraries may cause a mis-
representation of the suitability of Boost.Signals for a given 
platform/compiler.  Note that I did not evaluate whether or not it 
was possible/useful to factor these other Boost libraries out of the 
tests, however.

* Some of the tests never do any "assertions".  This makes them 
useless in automated regression testing.  I'd recommend either 
reworking them to make the proper "assertions" or to move them to the 
examples directory.

Design/implementation:

* I wonder if last_value, etc. should be promoted up to boost from 
boost/signal.  They seem to be useful concepts not tightly coupled to 
the Boost.Signals library.

* I wonder if signals_common.hpp should be demoted down to 
boost/signal/detail.  This helps distinguish the code as detail with 
out even opening the file.

* I wonder if some of the names used in Boost.Signals are too common 
for placing directly in namespace boost.

* The controlling state of the connection type seems a little off.  
It would be nice to have an inverse to non_controlling() so that we 
can do something like:

boost::connection con = sig.connect(foo).controlling();

instead of:

boost::connection con = sig.connect(foo);
con.set_controlling(true);

* It might be useful to have an overloaded connect() that takes a 
second parameter of type trackable.  This would allow an alternative 
to derivation from trackable, though at the expense of a more 
complicated interface.  The trade off might be worth it to some, 
though.

Bill Kempf