Boost logo

Boost :

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

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;


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


Boost list run by bdawes at, gregod at, cpdaniel at, john at