Re: [Boost-users] [signals2][review] The review of the signals2 library (formerly thread_safe_signals) begins today, Nov 1st

Subject: Re: [Boost-users] [signals2][review] The review of the signals2 library (formerly thread_safe_signals) begins today, Nov 1st
From: Fabio Fracassi (f.fracassi_at_[hidden])
Date: 2008-11-10 05:30:45

Hi,

I consider this submission a evolutionary development of the signals
library, so I'd like to focus the review questions on the parts that
changed.

Stjepan Rajko schrieb:
[...]
> 
> Here are some questions you might want to answer in your review (feel
> free to skip those that don't apply to your analysis):
> 
>     * What is your evaluation of the design?

* Automatic connection management:
This is great, even if it wasn't required for thread safety reasons, I
prefer using shared_ptr based connection tracking over boost::trackable,
since it enables me to track (and thus safely use) 3d party classes.

So on that note I would suggest to strike the "Unfortunately" from the
Auto Connection Management Design Rationale page, and emphasize the
benefit of the new design.

* Namespace signals2:
Under normal circumstances I would argue for keeping the name "signals"
for the namespace, because frankly its the best name for the concept.
But considering the real-world constraint of staying compatible with
code using Qt, any other name is much preferable. (I consider this a
deal-breaker, since using "signals" forces you to either patch boost, or
to educate your users of the Issue and have them deal with the problem)

* I haven't realy understood the connect_extended / extended_slot_type 
from the documentation.

>     * What is your evaluation of the implementation?

Just skimmed through it,

>     * What is your evaluation of the documentation?

I haven't done a comparison, but the Docu seems to be mostly the same as 
the original signals. Thus for the usecases that are documentated there 
its high quality, but I miss the same for the new features:

* I miss tutorial quality examples for some of the new features:
   ** Howto use boost::threads with signals2
   ** Howto use postconstructors/predestructors (and what for?)
   ** An example using signal::extended_slot_type

* The class reference pages (e.g.
doc/html/boost/signals2/connection.html) are missing from the TOC

* Do the portable syntax examples need to be supported (... so
prominently)? Maybe they could be moved to an appendix?

* The class/function reference pages format makes it hard to read.
   It would be nice if they used a stylesheet like doxygen does (see for
example this page:
http://xerces.apache.org/xerces-c/apiDocs-3/classDOMDocument.html , it
makes it much easier to see which function is being documented by any
given text.) (This nit is not specific to signals2, but while I'm at it ...)

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

Very useful, as it makes the current signal library more generaly usable.
Fixing the nasty Qt issue (even though its not boosts fault) is a great
maintainace boon, and getting rid of trackable makes it much easier to
"sell"

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

I use the previous version (thread_safe_signals-2008-03-12) on gcc-4.2
and MSCV-8.0 ( and shortly on 7.1). No problems there, even when using
it as drop in replacement for the old signals.

I have not encountered any performance problems, but neither did I check 
for them, and am moderately concerned about signals2 beeing too slow.

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

In-depth documentation reading, a quick reading of the source / examples
/ tests, a bit of usage.

>     * Are you knowledgeable about the problem domain?

Moderately so, I have used boost::signals before, I used Qt Signals, and
maintained a bigger project which used its own signaling mechanism.

I'm only moderately knowledgable in threading Issues.

> And finally, every review should answer this question:
> 
>     * Do you think the library should be accepted as a Boost library?
> Be sure to say this explicitly so that your other comments don't
> obscure your overall opinion.
> 

I have a tendency to yes, because even now it is better than what we 
have now (which is not saying that signals1 was bad!), on the other hand 
I think that the missing documentation is crucial.

Regards

Fabio