Re: [boost] [review] Fast track review of Boost.Utility/Singleton begins today

From: Pierre-Jules Tremblay (boost_at_[hidden])
Date: 2008-01-15 22:59:42

On Jan 14, 2008 12:10 PM, John Torjo <john.groups_at_[hidden]> wrote:

> * What is your evaluation of the design?
>

Not generic enough.  It addresses a number of specific problems (e.g. use in
MT contexts, order-of-initialisation fiasco, etc) but the author's design
choices are neither customizable nor overridable.  For instance, the design
could be made more flexible by allowing thread-safety to be specified as a
policy (with appropriate default) or by delegating the responsibility to the
wrapped type.  If BOOST_HAS_THREADS is defined, it looks as though I have no
choice but to use mutexed instantiation and/or access.  If I'm not
interested in synchronised access, then the lease interface is useless.

The solution to the OOI problem does not scale, and I would go as far as
saying that it doesn't work in practice.  In my experience, it's a constant
maintenance nightmare to try and keep the various slot assignments in the
correct order (I have to deal with a large number of singletons; yes that is
a problem in and of itself - this library will do nothing to discourage
it).  The only scheme I've personally had any success with, so far, involves
some form of reference counting, i.e. singletons that depend on other
singletons do so explicitly (by incrementing the other's refcount on
construction; the singletons are destroyed when their refcount goes to
zero).

Another reason why the proposed OOI solution fails in practice is that other
singletons are not a singleton's only dependency: client code ( e.g. objects
that reference the singleton) are dependencies as well and may affect the
order of destruction.  You just can't assume the singleton manager will be
the last thing to go - there may be other globals.  BTW I find the very idea
of being able to "force" singleton destruction quite scary, but I haven't
looked at the implementation details.  The following sentence is also cause
for concern: " Attempts to access an already destroyed Singleton instance
cause the instance to be re-created."  Is that true only for DLL usage?

My biggest problem with this design is that it is unsuitable for library
writers: types derived from typical singleton class designs, including
boost::singleton and friends under review, invariably give rise to
untestable code.  To make a singleton's client code testable involves
exposing some kind of overridable factory so that the singletonized type can
be replaced with a test mockup.  So far I have found this to be a hard
problem to solve in general.

Finally, this may constitute a minority opinion but I like to consider
global access and single instance as orthogonal concepts.  One may want to
ensure a resource is only instantiated once without necessarily providing
global access to that resource.

> * What is your evaluation of the implementation?
>

Unnecessarily complex as a consequence of the design.  If the functionality
was layered, the code would be more readable.

* What is your evaluation of the documentation?
>

It's OK although the DLL bit is unclear to me.

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

I won't use it, but I command the author's attempt to provide a library  for
such a controversial design pattern.

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

No.

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

I read the documentation, looked through the implementation and glanced at
the tests.

* Are you knowledgeable about the problem domain?
>

Yes.

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

No.

pj