Re: [boost] Re: Formal Review: Signals

From: Douglas Gregor (gregod_at_[hidden])
Date: 2002-02-20 12:16:05

On Tuesday 19 February 2002 06:19 pm, you wrote:
> 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".

Thank you, I'll add both.

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

That last parameter is a bit vexing, and deserves more documentation than it 
has. It's a hack to get around the lack of partial ordering of function 
templates. Consider:

template<typename> class A {};
template<typename T> void foo(T);
template<typename T> void foo(A<T>);
A<T> at;
foo(at);

Without partial ordering of function templates, the call on the fifth line 
will be ambiguous. However, one can do this:

template<typename> class A {};
template<typename T> void foo(T, long);
template<typename T> void foo(A<T>, int);
A<T> at;
foo(at, 0);

Even though the dumb compiler can't figure out that A<T> is better than T, it 
can figure out that int -> int is better than int->long, so it chooses the 
specialization.

I'll add this to the documentation.

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

There are three tests that fall into this category: dead_slot_test.cpp,  
deletion_test.cpp, and random_signal_system.cpp. The former two use 
Boost.Bind (this dependency could be removed without too much hassle). The 
random_signal_system test relies on the BGL to mimic the signal connection 
graph, and it would be horrible to rewrite all of that graph-handling code. 
Unfortunately, there are platforms (e.g., Borland C++) where the BGL does not 
work but Signals does. Maybe I can just document around the problem?

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

Egads, thanks. 

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

If anyone has a use for it, sure. I just couldn't find any reason to do this 
except within Signals.

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

Okay, will do.

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

I assume you're referring to 'connection' and 'trackable'?
If so, I think the long-term goal should be to split the Signals library into 
two: a Tracking library and a Signals library. Everything related to tracking 
connections would become a part of the Tracking library, which the Signals 
library would use instead of having its own mechanism. Then it might not be 
so unreasonable for 'connection' and 'trackable' to be in the Boost namespace.

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

I think I'd like to think about other alternatives to derivation before 
adding these overloads. I guess I'd prefer something like:

  sig.connect(track(some_function_object, a_trackable_object));

Then the notion of attaching additional trackable objects would be separate 
from the notion of connection. This goes back to the idea of having a 
Tracking library.

        Doug