Signals review

From: Darin Adler (darin_at_[hidden])
Date: 2002-02-21 20:52:40

I didn't have a chance to try out the Signals library, but I did read the
documentation and some of the code.

From my perusal of the documentation, I'd recommend that this library be
accepted to Boost. Along with Boost.Function and Boost.Bind, it provides an
excellent way to implement model/view separation and avoid getting tangled
in virtual functions and multiple inheritance.

Here are my comments:

================

A comment in <boost/signal.hpp> says "The unusable class" just before the
definition of a class named unused. Strangely, it seems that both
boost::detail::signals::unused and boost::detail::signals::unusable exist;
is this intentional?

The beginning of the tutorial doesn't say when or why boost::signal2<> is
better than boost::signal<>. I think you should mention right at the outset
that the numeric form is needed if you are providing the additional template
parameters which are discussed below. Otherwise, the question, "Why force
the programmer to count?" comes up.

The documentation says that signals are typically "class member variables".
This confused me for a moment. You mean "class data members". The C++
standard never calls data members "variables", so you should probably avoid
it too. I would say more explicitly that signals are typically data members
within non-copyable objects.

I'd strongly prefer to have a scoped_connection class rather than the
"controlling" family of member functions on a connection. But I may have
missed some reason that the proposed design is better.

The tutorial documentation about named slot connections doesn't mention that
connecting a new slot with an already-used name causes the old slot to be
disconnected, and I think it should.

I think the coverage of "user-defined slot types" in the tutorial needs to
be improved. The example is trivial and does not demonstrate how this
feature allows "other callback classes to integrate with Boost.Signals
seamlessly". The tutorial also doesn't say if using this feature interferes
with other features, like the automatic disconnection of a slot that
involves a trackable object.

If a programmer would need to use boost::detail::signals::unusable to
implement boost::last_value properly, then unusable should probably be
public, and not in boost::detail.

I think that last_value and visit_each should either be put in some suitable
library elsewhere in Boost or made private. Since visit_each is already in
its own top-level header, this is only a documentation issue. In the case of
last_value, it seems that unusable might also have to be made public, and
made a primitive in Boost rather than part of the signals library.

The global names from this library, "signal", "slot" and "connection", seem
broad enough that perhaps they should *all* go within a boost::signals
namespace. If the names stay at the top level in the boost namespace, then I
suggest moving the header files up out of the signals directory too, so the
directory structure matches the identifier structure. While this might make
some clutter for people trying to look at the Boost directory, it makes it
much easier to remember what to type when writing include statements.

I think that providing combiners for functions returning bool with && and ||
semantics should be considered. The && one can be thought of as "stop on
first false" and the || one "stop on first true". The || one comes up
incredibly often in event handling, one of the most common signal handling
application areas.

Perhaps the interface for finding trackable objects inside Boost.Bind should
be documented. It's part of the public interface of Boost.Signals, in a way,
no?

================

Perhaps I'll have a chance to use the library later in the review period; if
so, I'll post my additional comments after that.

    -- Darin