[boost] [signals2][review] signals2 review results

Subject: [boost] [signals2][review] signals2 review results
From: Stjepan Rajko (stjepan.rajko_at_[hidden])
Date: 2008-11-20 00:25:59

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