[boost] [signals2][review] Fwd: Signals2 Review

Subject: [boost] [signals2][review] Fwd: Signals2 Review
From: Stjepan Rajko (stjepan.rajko_at_[hidden])
Date: 2008-11-03 12:59:57

I am forwarding the following review submitted by Jesse Williamson:

---------- Forwarded message ----------
From: Jesse Williamson <jesse_at_[hidden]>
Date: Sun, Nov 2, 2008 at 5:09 PM
Subject: Signals2 Review
To: Stjepan Rajko <stjepan.rajko_at_[hidden]>

* Do you think the library should be accepted as a Boost library?

       Yes.

* What is your evaluation of the design?

       Note: I only read the tutorial, and wrote my own examples as I
       followed along.

       I think this is a very nicely designed library!

       I like the collector interface, and the general library behavior.

       Note: I was going to suggest some syntactic sugar with operator
       overloading, but when I tried it I quickly discovered that
       there's a problem in determining the proper return type for
       something like + or +=. It isn't really needed anyway-- and,
       I also have since discovered that the documentation already
       addresses this!

* What is your evaluation of the implementation?

       I did not examine the implementation. For the most part, things
       worked as I expected after reading the tutorial examples.

* What is your evaluation of the documentation?

       Good!

       The tutorials are on the whole very good, and I came through them
       feeling like I had a good working understanding of the library.

       I have a few suggestions and perhaps issues, which I discuss
       below in more detail. I would like to see more explit examples
       in a few of the tutorial examples, especially those concerning
       connection management.

* What is your evaluation of the potential usefulness of the library?

       Event patterns show up in many applications, and a thread-safe
       version of the long-standing Boost::Signals library would surely
       greatly extend its usefulness.

* Did you try to use the library? With what compiler? Did you have
any problems?

       gcc 4.2.1 on AMD x86-64, I had no problems using the library,
       although I did run into a compiler issue with the examples as
       written in the tutorial, it was fixed in the packaged example
       file.

* How much effort did you put into your evaluation? A glance? A
quick reading? In-depth study?

       I spent a day reading the tutorial and working through the
       examples.

       I had few problems writing my own versions of the tutorial
       programs, and feel like I came out of them with a good working
       understanding of the library's use. I believe I worked through
       all of them.

* Are you knowledgeable about the problem domain?

       Kinda. I've used a fair number of event-driven frameworks, but
       beyond peeking at it, I've never used the Signals library before.

       My review is from evaluating the tutorial only.

---------------

Some comments and thoughts, mostly written as I went through the tutorial.

Documentation:
-------------

* Perhaps you should point out the location of the FAQ earlier in the
synopsis. I found it at the end, but may not have if I'd been in a
hurry.

* ToC and main page:

The first thing that I notice is that I can infer from the original
copyright notice that Douglas Gregor's library appears to have been
stable for a really long time! Nice work!!

* Introduction:

Is it really important that we know Signals2 was formerly known as
thread_safe_signals? You might consider pointing this out in the
introduction text itself, instead, since it becomes redundant and is
more-or-less superfluous information.

The introduction gave me a clear idea of what the library is for, and
the general abstraction it presents. I've worked with a fair number of
event-based systems, and so it already sounds like familiar territory
and
I felt like I got a good sense of what Signals2 is for.

Tutorial:
--------

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

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

* 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? :>)

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.

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

* "Disconnecting Equivilent Slots" example:

I didn't feel that the example program for "disconnecting equivilent
slots" fully illustrated the library concept. Perhaps that should be
made more clear in the example by disconnecting via a pointer to the
same function, instead of by using the same identifier that was used
to connect(), something like:

       // ...
       boost::signals2::signal<int (int a, int b), aggregate_values<
vector<int> > >  sig;

       sig.connect(&add);
       sig.connect(&half);

       // results of half() seen:
       {
               vector<int> results(sig(5, 5));
               dump(results);
       }

       // Disconnect half() by using an equivilent slot:
       int (*ptr_to_half)(int a, int b) = half;
       sig.disconnect(ptr_to_half);

       // results of half() not seen:
       {
               vector<int> results(sig(5, 5));
               dump(results);
       }
       // ...

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

Also, maybe it would be nice if the signal_type typedef could be
included closer to its actual use in the example?

* 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));

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

* Comment on "Passing Slots" example:

In the "passing slots" example, Button::f() never appears to get
bound. A complete example would be nice.

...on the whole, I enjoyed this tutorial and feel like I can star
working with the Slots2 libray! Thanks.