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