Boost logo

Boost :

From: Gary Powell (powellg_at_[hidden])
Date: 2002-02-26 13:10:28

So far I haven't had time to run/build this code, but after reading
it I do have some general and some specific comments.
(Plus some comments that will, I'm sure, expose my lack of
knowledge about this topic.)

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

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

was the call that generated the side effect.

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.

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

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.

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

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.

I vote a "qualified" maybe vote. After the auto_ptr issues are
addressed as well as the question about *iter's side effects.
(What a waffle!)

That's all the time I have for this now. Maybe more comments


powellg a t amazon d o t com

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