Re: [Boost-users] [signals2][review] Fwd: Signals2 Review

Subject: Re: [Boost-users] [signals2][review] Fwd: Signals2 Review
From: Frank Mori Hess (frank.hess_at_[hidden])
Date: 2008-11-03 14:44:18

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Monday 03 November 2008 12:59 pm, Stjepan Rajko wrote:
> * Introduction:
>
> Is it really important that we know Signals2 was formerly known as
> thread_safe_signals? 

No, it's not particularly important.  Mostly I put it in because it was still 
called thread_safe_signals when I originally requested a review, and wanted 
to avoid confusion.  I'll remove it if the library gets accepted.

> * Do we need "beginner", "intermediate", and "advanced"?
>
> I'm not sure why I'm cautioned that the tutorial is not meant to be
> read linearly, especially since the tutorials typically (and
> expectedly) progressed from basic to advanced material. Readers tend
> to pick and choose what they need anyway, and it seemed more visually
> cluttering than anything. I just followed everything linerally, and
> did not seem to have many problems.
>
> On the other hand, if this is essentially how the documentation has
> been structured for years, it's unlikely there's any need to change
> it.

Yes, that structure (and the vast majority of the tutorial) comes directly 
from the original Boost.Signals documentation.

> * Compiler versions and preferred-form table:
>
>        I wonder if, instead of maintaining a list of which compilers
> do support the preferred syntax, it would not be easier to just
> maintain a list of those which do not support it, especially since it
> looks to be that only broken compilers don't.

I actually completely ripped that table out recently, since it really is only 
based on compiling the original Boost.Signals library, and I've never updated 
it.  However, maybe I should bring back the "unsupported compilers" part of 
it, since they definitely won't compile signals2 preferred syntax.

A larger question might be whether the "portable syntax" should be deprecated 
entirely and removed from the tutorial.  I really don't know if any of those 
old compilers will even compile signals2 in the "portable syntax".

> * Slot ordering examples:
>
> In the intermediate example of ordering slots, I wasn't able to change
> the order my signal was displayed after reading the material a few
> times.
>
> It always came last, no matter what order I added it in. I was able to
> do it, but it was by using the numeric ordering I'd learned in the
> beginner section.
>
> In my own test program, I tried:
>
>        // ...
>        GoodMorning gm;
>        HelloWorld hello;
>        Newline nl;
>
>        sig.connect(gm);        // confused as to why this always comes last
>        sig.connect(2, nl);
>        sig.connect(1, hello);
>        sig.connect(0, gm);     // ok, clear why this goes first
>        // ...
>
> ...after playing around a while, I understood what the intermediate
> example was explaining, and it made sense.
>
> Maybe it's a pitfall of reading the tutorial out of the recommended
> order, but the beginner section didn't really seem to explain to me
> clearly that sig.connect() is actually adding groups and not
> "individual" signals.
>
> (Or, perhaps I'm still confused about how it works? :>)

Yes, it could be clearer.  What is really happening is there are two special 
slot groups for ungrouped slots.  One has all the ungrouped slots connected 
with "at_front" and the other has all the ungrouped slots connected 
with "at_back".  The ungrouped "at_front" slots are always executed before 
any grouped slots, and the ungrouped "at_back" slots are always executed 
after all grouped slots.  This behavior is taken from the original 
Boost.Signals.

> The section where we deal with return values just feels to me like a
> logical extension of the behavior the beginner tutorial has just
> discussed, rather than as a seperate tutorial with significantly more
> advanced behavior-- it seemed natural to want to return values next.

My guess was Doug made those beginner/advanced assignments based on how 
commonly someone would want to use a particular feature.  I'd guess wanting 
to pass arguments from the signal invocation to the slots is far more common 
than wanting to receive a return value at the signal invocation from the 
slots.

> * Blocking tutorial:
>
> I got a suprise playing with the blocking section when an assertion
> was triggered:
>        // ...
>        boost::signals2::signal<string (string a, string b)> sig;
>
>        boost::signals2::connection c = sig.connect(&revcatgoat2);
>
>        if(c.connected())
>         cout << *sig("a", "b") << endl;
>
>        // ...
>
>        {
>                boost::signals2::shared_connection_block block(c);
>
>                if(!c.blocked())
>                 {
>                        cerr << "something is wrong\n";
>                        throw;
>                 }
>
>                cout << "You shouldn't see this: ";
>                cout << *sig("a", "b");         // SUPRISE abort()!
>                cout << endl;
>        }
>        // ...continue, expecting output when block goes out of scope...
>
> ...The tutorial didn't say anything about that happening. Perhaps I
> was misusing the library..? It worked fine when I checked for
> c.blocked() again before trying to call sig().

Hmm, yes I should add a bit more about the perils of optional_last_value in 
the tutorial.  What's happening is, the signal is returning an empty 
boost::optional because the signal's only connection is blocked when you 
invoked it.  Then attempting to dereference the empty optional produces the 
assert.  So what you would want to do is store the signal's returned optional 
and check that it is non-empty before dereferencing it.

> * Connection Management example:
>
> I got a small (but pleasant) suprise when I tried out the connection
> management:
>
>  {
>        messageSignal deliver;
>
>        messageReciever *mr     = new messageReciever("message reciever
> one");
>
>        deliver.connect(boost::bind(&messageReciever::show, mr, _1));
>        {
>        messageReciever *mr2    = new messageReciever("message reciever
> two"); deliver.connect(boost::bind(&messageReciever::show, mr2, _1));
>
>        deliver("message");
>        }
>        deliver("message"); // SUPRISE: no problem
>  }
>
> ...after reading the tutorial section, I had expected the second
> deliver() call to segfault, but instead it worked nicely. I'm not
> really going to complain about this, but I'd expected something
> differently from the tutorial's explanation that it was expected to
> segfault.
>
> It seems reasonable to expect this behavior, given the explaination
> about shared_ptr<>. Indeed, calling delete explicitly produced mangled
> output. Perhaps extending the example to show an explosion and how
> track() gets around it would be worthwhile.

The tutorial does say in the text that the newsMessageArea object is 
destroyed.  However, yes it would be clearer if it spelled it out in the code 
as well.

> * Automatic Connection Example
>
> Problem: There seems to be no get() for NewsMessageArea in the
> automatic connection management example, and the signature with the
> application operator appears to probably not be correct. I had to use:
>
>        // Note second parameter to slot_type ctor:
>        deliver.connect(messageSignal::slot_type(&messageReciever::show,
> mr2, _1).track(mr2));

The get() is from shared_ptr.  Did you notice how in the connection management 
snippet, newsMessageArea has been changed from a raw pointer into a 
shared_ptr?  If you bind a copy of the shared_ptr to the slot (get rid of the 
get()) then the shared_ptr will never expire due to the copy living in the 
slot.

>
> * Compiler complaint in "Document-View" example:
>
>        button.doOnClick(&printCoordinates);
> ...requires:
>        button.doOnClick(&Button::printCoordinates);
>
>        ...the example included in the package did not have this issue.
> (I implemented my own in working through the tutorial.)

In the tutorial, printCoordinates is a free function.  You must have 
implemented it as a member of the Button class?

> * Comment on "Passing Slots" example:
>
> In the "passing slots" example, Button::f() never appears to get
> bound. A complete example would be nice.

Hmm, yes I think 

void f(Button& button)
{
  button.doOnClick(&printCoordinates);
}

could just be replaced with something like

Button button;
//...
button.doOnClick(&printCoordinates);

since f() isn't used for anything.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFJD1SS5vihyNWuA4URAsiJAJwPEG4tZaRRNuoeoUlthsOUU873CgCgzpfB
cBDIRt/gz7crhSe+k9VkNmU=
=HxYi
-----END PGP SIGNATURE-----