Re: [boost] Re: Formal Review: Signals

From: Douglas Gregor (gregod_at_[hidden])
Date: 2002-02-26 16:34:09

On Tuesday 26 February 2002 01:10 pm, you wrote:
>  In signals/last_value.hpp
> In the function, last_value<void>
>    replace *first++; with ++first;
> The result is unassigned and the post fix increment unnecessary.
>
> My preference is to re-write the basic last_value function like this:
>
> last_value(iterator first, iterator last)
> {
>     assert(first != last)
>     iterator prev(first++);
>     while (first != last)
>        prev = first++;
>     return *prev;
> }
>
>   In case there is some major overhead with calling operator*();
> Or am I missing something. Do the "iterators" have state, and therefore
> depend on the correct number of calls to operator*()? This is not at
> all clear from the code. (The tutorial hints at this.)

Just to make sure we're on the same page: the first time operator* is invoked 
on one of these iterators, the slot is called with the set of arguments given 
to the signal and the return value is cached. Any time after the initial 
dereference and prior to an increment, operator* returns the cached value. If 
the iterator is never dereferenced, the slot is never called. 

> Ok, I just read the doc's and I still think its a misuse of operator*().
> IMO it would be more clear use of an iterator if
>
>    (*iter++)();
> was the call that generated the side effect.

What is your reaction to transform_iterator from the iterator adaptors 
library? It also uses operator* to invoke a function. In any case, here's my 
justification:

From the point of view of a combiner, the sequence of slot calls is merely a 
sequence of values. We already have a concept that describes a sequence of 
values: an input iterator sequence. By reusing that concept, we are 
automatically able to reuse any operation defined on input iterator 
sequences, and the STL is full of those.

> -------------------------------------------
> In the signalsN.hpp files there is a mismatch in naming conventions.
>
>    T0 arg1_type, T1 arg2_type.....
> IMO The typenames T0 -> TN should be T1 -> T(N-1) It should make reading
> the error messages easier. (What compiler errors!?! Unheard of.) To match
> the argument naming convention. The documentation seems to follow
> my suggestion.

Oops. Thank you.

> -------------------------------------------------
> In signal_base.hpp
>
>    Shouldn't the constructor for signal_base be written as follows?
>
>    signal_base(const compare_type & comp)
>
>    : impl(0)
>
>    {
>        std::auto_ptr<signal_base_impl> tmp(new signal_base_impl)
>        impl = tmp;
>    }
> --------------------------------------
> Same sort of complaint auto_ptr complaint for slot_call_iterator.hpp and
> slot_call_policies.

Good catch. Thanks.

> ---------------------------------------
> And again in signal_base_impl::connect_slot
>           basic_connection * con = new basic_connection;
> looks like it could use an auto_ptr as well.
>   then call
>   safe_connection.reset(con.release() ); or else make
> safe_connection take the auto_ptr.

No exceptions can be thrown between the creation of 'con' and the 
'safe_connection.reset' (note: the default constructor for connection will 
not throw), but it's always good to get rid of spurious raw pointers :)

> And again in ::slot_disconnected()
>
> and again in slot.cpp create_connection()
>
>   std::auto_ptr<slot_iterator> slot = reinterpret_cast<....>(blah blah);
>
> and then you can toss the explict call to delete slot;

Okay.

> ----------------------------------------------------------
> Do the slots ever shrink? I can see where things are disconnected,
> but it doesn't look like the container contracts until its destructed.
> Or am I missing something here?

Yes, they do. When a signal and slot are disconnected, the slot gets notified 
of the disconnection via trackable::signal_disconnected, which erases the 
connection from the list of connections known to the slot.

Note: I see that trackable::signal_disconnected could also use an auto_ptr to 
be a little safer.

> ----------------------------------------------------------
> In the get_real_signal_impl would using a typelist made this
> code any simplier? Seems like the right application for one.
> (At first it looked like a tuple application, but I think you
> can get by with a typelist.)  Just some musing over the design.

I'm not sure that using typelists would make it much simpler, but I might be 
missing something obvious. That ugly implementation was inherited from 
Boost.Function, where I ended up coming up with that ugliness to make it 
portable (grumble, grumble).

> ------------------------------------------------------------------
> I must be missing something because "vist_each" appears to be a misnomer.
> It's just
>
>     bind(visitor, _1);
>
> And as such doesn't deserve its own boost level exposure. Is the intent
> that user's will write their own specialization to make this work?
> (Call me confused.)

Yes, users would need to write their own overloads to work with their own 
function objects. For instance, Boost.Bind has overloads for visit_each so 
that Signals and Bind integrate nicely. I'll be doing the same for Lambda at 
some point...

> ----------------------------------------------------------------
> Shouldn't the vector of <trackable *> be smart_ptr's? It's not clear to
> me who owns this memory, and therefore when it can be destructed.

The user owns the memory. All of the trackable pointers are extracted from 
user objects that are found by digging through the function object given by 
the user. Note that even though we don't have any control over this memory, 
we will know if one of these objects is destroyed because we have a 
connection in common with all of them.

Thanks for the detailed comments.

        Doug