[signals2][review] signals2 review results
** signals2 review results ** I am pleased to announce that the signals2 library by Frank Mori Hess is accepted into boost, pending some conditions outlined below are met. I would like to thank the author for his work on this library, and for his very responsive participation in the review. I would also like to thank Franz Alt, Jean-Cristophe Benoist, Vicente Botet, Fabio Fracassi, Johan Råde, Ravikiran Rajagopal, Andrew Webber, and Jesse Williamson for submitting reviews, and Terry Golubiewski, Michael Marcin, and Hansjörg for additional comments regarding the library. There were 5 votes cast for the acceptance of this library, and 3 against accepting the library at this time. The reviewers that were against immediate inclusion of signals2 suggested that the library should be accepted when several stated problems are addressed adequately. Overall, the library was regarded as a much needed thread-safe successor of Boost.Signals. Those that have tried switching from Boost.Signals to signals2 have found the switch rather painless (at least in the cases that don't involve the Boost.Signals trackable interface), and some reported using signals2 and/or its previous versions in production code. Some additional improvements that were mentioned are an automatic connection management system based on shared_ptr, and a change of namespace (which avoids problems with QT). Additionally, many requests were made for improvements in the documentation, as well as some improvements in thread-related testing and some possible changes to the interface. Frank has addressed many of the issues in the review, and in many cases offered concrete steps to address the problems. Following is an overview of the most prominent issues that were raised. These issues must be addressed before this library is included in the trunk and subsequent releases: ** Boost.Signals -> signals2 transition ** There needs to be a concrete plan of migration from the current version of Boost.Signals to signals2. The almost-decided-on route is to have both Boost.Signals and signals2 co-exist in Boost for a few releases, with Boost.Signals getting deprecated. From reading the reviews, it looks like no one has a problem with this, so it seems like it's just a matter of an agreement between the library maintainers that gets posted in the documentation / on the lists. The bigger part of the problem is to make sure that users of Boost.Signals can transition to signals2 as easily as possible. Given the reports so far, this process is indeed easy, with the biggest exception being the removal of the trackable interface in signals2 (which was removed because it is not thread-safe). Frank has indicated that he will put it back into the library and make it available for single-threaded signals as a way to ease porting for Boost.Signals users, which sounds like a great step. My biggest concern here is that the signals2 interface is "gotten right" (with respect to compatibility with Boost.Signals) by the time it is included in Boost, so that porting is both easy and a fixed target. Given how easy people have found porting their code so far, it looks like restoring the trackable interface might is all that is needed. I would discourage any further breaking changes to the interface (one thing mentioned was dropping some of the template parameters), but I also recognize the opportunity to improve the interface in response to the review. Any signals2 interface changes that significantly break compatibility with Boost.Signals should be considered very carefully (even if Boost.Signals will stay on for a while), and if possible, counterbalanced by a compatibility layer put in to ease the transition for Boost.Signals users. ** Documentation ** Pretty much all reviewers noted the documentation as lacking with regard to thread-safety aspects of signals2 and other added functionality. Frank has clarified many of the questions that came up during the review, and outlined a plan of integrating the requested information into the documentation. Specific items are outlined in my notes at the end of this mail. ** Tests ** Some reviewers also noted a lack of tests for thread-related functionality. I agree that whatever can be reasonably tested should be tested. Frank has suggested he will try to adapt some of the Boost.Thread tests where adequate. As stated, the above issues should be addressed before moving the signals2 library to the boost trunk. I don't anticipate significant problems because: * the trackable interface has already been brought into signals2 at one point in the past * all documentation related questions have been clarified during the review, and Frank's suggestions for updating the documentation were agreeable * Boost.Thread tests should provide some insight into how signals2 thread-safety could be more thoroughly tested. Also, several reports have been made of signals2 already being successfully used in production settings, which speaks to its reliability. I strongly suggest that Frank make an announcement when the changes are completed, so that those interested can make sure that their concerns have been adequately addressed. Given Frank's record as a very responsive maintainer of this library in the past, and his commitment towards the peer-review process (he insisted on having this library reviewed even though it could have technically been included with Doug Gregor's approval as an upgrade to Boost.Signals), and considering that one of the reviewers has already offered to review changes to the library, I anticipate that a de facto mini-review of the library will happen at that point even though I am not requiring one formally. Finally, below are some notes on suggested changes I extracted while reading through all of the reviews. Square brackets contain initials of some of the reviewers that mentioned the issue, and double curly braces my personal comments. This is by no means a record of everything that has been mentioned, but perhaps the author will find it useful while making the post-review changes. To be clear, the below changes are not required for the inclusion of signals2 into the boost trunk, except as they pertain to the required changes outlined above. -- Implementation -- ---- * source code could be formatted more clearly [FA] * address efficiency problems where signals2 is significantly slower than signals [FA] {{ while efficiency is important, and ideally signals2 efficiency for single-threaded use cases converges to that of Boost.Signals, this is one area where improvements can painlessly continue to happen after the integration of signals2 into the trunk (as long as they don't require interface changes), so I don't consider this a priority at this point }} * clean macro names used with Boost.Preprocessor [VB] -- Interface -- ---- * portable syntax - either establish that the syntax works (and on what compilers) or consider deprecating [JW] * use Boost.Parameter for signal template parameters due to so the user doesn't have to specify default template parameters. [VB][FA] {{ if there are any arguments against changing the signal class template to use Boost.Parameter directly, you could still provide a metafunction that uses Boost.Parameter to provide the signal type. This would allow, e.g.: signal_type<void(int, int), mutex_type<signals2::mutex> >::type my_signal; }} * use template parameters such as single_threaded or multi_threaded instead of the mutex type. [vb] {{ it should be possible to allow the Mutex parameter to be either a mutex class or a special (or policy) type that maps to a particular mutex }} {{ this suggestion was given in conjunction to a proposal for allowing external locking mechanisms, but it seems like external locking should be possible with what the library already offers - perhaps an example of this would be useful }} -- Documentation -- * update code examples in tutorial to contain fixes already present in the package [JW] {{ if you aren't already, consider using the quickbook import facility to keep the compiled code snippets and docs in sync }} * FAQ should be mentioned earlier / information in the FAQ should be more visible [JW][VB] * while the Beginner/Intermediate/Advanced designations have been found useful [FA], the tutorial has also been found readable beginning to end without much use for the designations [JW] * clarify call ordering between grouped / ungrouped slots [JW] * clarify optional_last_value [JW] [AW] * clarify the way slot::track works [VB][JW] {{ the documentation says we "pass the shared_ptr to the slot via its slot::track method before connecting", but the clarification given to Jesse Williamson also explains that the *pointer* held by shared_ptr is passed to the slot_type constructor. This seems easy to trip over - some syntactic sugar that avoids using the shared_ptr twice in the expression in two different ways might be helpful }} * offer / point to some documentation on, and provide an example for postconstructible / predestructable / deconstruct_ptr. [FA][AW][VB][FF] * document / provide example for extended_slot_type [FA] [FF] * expand the documentation of various mutexes that can be used and their properties [VB] also in regards with co-use with Boost.Thread [FF] {{ I can find the mutexes explained in the reference documentation, but an overview at the tutorial level would be nice }} * expand discussion on thread-safety: ** how exactly is signals2 thread-safe? [JB][RR] ** explain behavior in various threading circumstances [JB] * mention the benefits of the new connection management based on shared_ptr over old trackable interface (e.g., allowing non-intrusive use with 3rd party classes) [FF] * consider making the portable syntax examples less prominent [FF] -- Testing -- * add multi-threading tests [JB] * include performance tests [VB] Again, thanks and congratulations to Frank Mori Hess, and thanks to all of the reviewers. Kind regards, Stjepan
On Thursday 20 November 2008 00:25, Stjepan Rajko wrote:
I am pleased to announce that the signals2 library by Frank Mori Hess is accepted into boost, pending some conditions outlined below are met. I would like to thank the author for his work on this library, and for his very responsive participation in the review. I would also like to thank Franz Alt, Jean-Cristophe Benoist, Vicente Botet, Fabio Fracassi, Johan Råde, Ravikiran Rajagopal, Andrew Webber, and Jesse Williamson for submitting reviews, and Terry Golubiewski, Michael Marcin, and Hansjörg for additional comments regarding the library.
Thank you to Stjepan for managing the review, and to everyone who took the time to write reviews and provide feedback.
As stated, the above issues should be addressed before moving the signals2 library to the boost trunk.
I'll post a RFC to the boost devel/users lists once I'm done addressing the issues raised during the review, to give people an opportunity to provide more input before it goes into a release. One minor point: my understanding is code in the trunk doesn't go into releases until it is manually copied into the release branch. So I think the important move wouldn't be the one from sandbox to trunk, but copying the library into the release branch.
On Thu, Nov 20, 2008 at 7:01 PM, Frank Mori Hess <fmhess@speakeasy.net> wrote:
I'll post a RFC to the boost devel/users lists once I'm done addressing the issues raised during the review, to give people an opportunity to provide more input before it goes into a release. One minor point: my understanding is code in the trunk doesn't go into releases until it is manually copied into the release branch. So I think the important move wouldn't be the one from sandbox to trunk, but copying the library into the release branch.
I was unsure on this issue myself, so I went with the more conservative decision. This was partly because I was mirroring previous reviews that had similar conditions (but those were before the current release process), and partly because every once in a while I see a discussion on the list that tries to pin down how stable or unstable the trunk is supposed to be, and I don't recall seeing a hard decision on the issue. My concern was that boost users or developers that use the trunk would have to deal with potentially frequent post-review changes should they choose to use signals2. I think working in a branch (http://svn.boost.org/svn/boost/branches/) would definitely be fine (this would be better than the sandbox because you could integrate with the boost structure - from what I remember branching all of boost into the sandbox is frowned upon). Could you do that until you feel like you have a relatively stable and reliable implementation? (depending on what changes you plan to make to the implementation, that might not take much). Another option might be to just create a signals2 directory in the trunk boost and libs directories, and those that want to get signals2 with the trunk can do an svn switch to a version in the branch (then they can latch on to a particular version, or the head, and they are very conscious of the fact that they are working with a library that might be going through frequent changes). Sorry if I'm making too much of this, perhaps one of the moderators can chime in and say that using the trunk is a non-issue and I'll happily shut up :-) Best, Stjepan
----- Original Message ----- From: "Stjepan Rajko" <stjepan.rajko@gmail.com> To: <boost@lists.boost.org>; "boost users" <boost-users@lists.boost.org>; <boost-announce@lists.boost.org> Sent: Thursday, November 20, 2008 6:25 AM Subject: [boost] [signals2][review] signals2 review results ** signals2 review results **
I am pleased to announce that the signals2 library by Frank Mori Hess is accepted into boost, pending some conditions outlined below are met.
Hi, I expected at least a mini review after library modification. Anyway congratulations Franz for the reviev result. I would like to make some suggestion to improve the review management: * I had notice only at the end of the the review that the review tokes place on two mailing lists (devel ans user). Maybe this is usaual on Boost reviews but I was not aware. It will be more transaparent if all the reviews were posted to the same mailing list, maybe a specific one should be created. * From the 5 committing reviewers making this review possible only 3 made a review and 2 of them late. I'm wondering if this new rule must be preserved as the review can be accepted without the commiting reviewers review. * There were some reviews that came into this list by the intermediation of the review manager with a delay between the posting of the reviwer and forward from the RM. One negative review posted the 4th and reveived in this list the 11th other positive posted the 2nd and received in this list 3rd. I think that the review manager should not encourage the reviewers to send the reviews to himself. This will avoid this kind of delays. So I purpose that only the reviews sent to this single mailing list must be taken in account. * Even if the review was over the 10th there were 2 accepting reviews comming from the commiting reviewers just some hours before the review result annoncement 19th. I think that the review manager must state clearly when the review is over and do not accept any review after. This do not means that the RMcannot change this date, but annnce it clearly. I hope this will help to improve future reviews, Vicente
I would like to make some suggestion to improve the review management:
<valid points snipped> Whilst I understand your points, unlike our day job, people do have other more pressing commitments and, for many, the extensive effort given to reviewing boost libraries is time given in an ad-hoc manner. Whilst it is good to have the timetable and a specific method for submitting reviews, I don't think we need to go further and be more regimented. The existing flexibility is a positive part of the process, IMHO. [ I do have problems with some libraries actually getting to the point of review before interest, maturity of interface or implementation are right, but that doesn't apply here and has been covered in previous discussions.] I'm grateful to all who devoted time to this library and I think Stjepan was flexible and handled the review very well. Paul
On Fri, Nov 21, 2008 at 6:40 AM, vicente.botet <vicente.botet@wanadoo.fr>wrote:
* I had notice only at the end of the the review that the review tokes place on two mailing lists (devel ans user). Maybe this is usaual on Boost reviews but I was not aware. It will be more transaparent if all the reviews were posted to the same mailing list, maybe a specific one should be created.
A specific maillist for reviews strikes me as an excellent idea. In Joels' review of his Phoenix 2.0 library there were some outstanding contributions from people I don't see posting very often, but who apparently feel moved to do important stuff like reviewing. These comments did much to give me some historical context, and understanding of structure, alternatives and implementation decisions. These will inevitably be lost in the general background noise of on going developments, and it would be nice to have a clear place for these review comments. - Rob.
participants (5)
-
Frank Mori Hess
-
Paul Baxter
-
Robert Jones
-
Stjepan Rajko
-
vicente.botet