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