[boost] [synapse] Review

Subject: [boost] [synapse] Review
From: David Sankel (camior_at_[hidden])
Date: 2016-12-12 20:19:23

On Fri, Dec 2, 2016 at 9:19 AM, Edward Diener <eldiener_at_[hidden]>
wrote:

>
> For your review you may wish to consider the following questions:
>
>      - What is your evaluation of the design?
>

The design looks reasonable.

>      - What is your evaluation of the implementation?
>

I found the use of typedef's to functions returning inline struct
definitions to be overly clever and cryptic to new C++ developers. I also
found the lack of static type checking in certain places to be
objectionable.

The unusual formatting is an important subjective turnoff to potential
users. Presumably the goal of the library is to enable users to get the
functionality they've been missing. The hurdle of a new formatting style to
get used to goes against this goal. Fortunately, this is something easily
remedied.

>      - What is your evaluation of the documentation?
>

Looked reasonable.

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

Given the prior existence of Boost.Signals I wouldn't find this library to
be useful.

There is an important aspect to this review that I want to call out. A
signal is a "vocabulary type" in that it is pervasive through systems and
frequently used in public interfaces. One of the most important qualities
of a vocabulary type is that there be only one of them. Imagine the
confusion if we had one thread-safe shared pointer class and another one
optimized for single threaded usage, or if we had an suite of different
variant types with different tradeoffs.

No, there should only be one signal type and Boost.Signal made the correct
choice of using a so-called intrusive design.

     - Did you try to use the library?
>

Nope.

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

I spent a few hours on the review. A small amount of time was reading the
documentation and a much longer amount of time was spent pondering the
implications of incorporating the library into Boost.

     - Are you knowledgeable about the problem domain?
>

Yes. I've used Qt signals, Boost.Signals1, and Boost.Signals2 extensively
in various projects.

>      - Do you think the library should be accepted as a Boost library?
>

No.

-- David Sankel