RE: [boost] Re: Formal Review: Signals

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

   (*iter++)();
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
slot_call_policies.

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

  Yours,
 -gary-

powellg a t amazon d o t com