I am forwarding the following review submitted by Jesse Williamson: ---------- Forwarded message ---------- From: Jesse Williamson Date: Sun, Nov 2, 2008 at 5:09 PM Subject: Signals2 Review To: Stjepan Rajko * 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 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 > 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.