Re: [boost] [Formal review] Singleton library

From: Dan Rosen (dan.rosen_at_[hidden])
Date: 2005-05-09 21:41:32

Pavel,

Overall, I do not consider the library ready for acceptance into
Boost. My review follows:

Jason's Singleton library would fill a clear niche, and the demand for
a flexible implementation such as this is obviously high. I think this
particular implementation has some novel ideas, particularly
"multiton," which I know I've had to implement before, but have never
seen anybody codify as a pattern per se. I also think that Jason had a
good jumping-off point in starting with Andrei Alexandrescu's
Loki::SingletonHolder. However I think Jason's work suffers from a
number of problems, both conceptual and concrete.

I'll rattle these off in no particular order:

1. Creator policy is redundant with the standard Allocator concept, I
think. Though the standard Allocator concept has some subtle, tricky
semantics, I think it's nevertheless worth using: that would permit
interoperability with existing allocator implementations.

2. The auto_reset struct has some conceptual issues. The biggest of
these is that the scope of the auto_reset object, if I understand it
correctly, can exceed the lifetime of the variable it references. If
such an object is strictly necessary, I believe it should own the
object it references. Another minor nit: the template should be
parametrized not only on the variable type, but also the reset value
type. And lastly, the documentation contains implementation details in
the interface.

3. The leaky_lifetime class is indicated as "not recommended for
general use." I tend to disagree: in production code I've had
experience with, it's been very appropriate to manually control the
order of destruction of singleton resources, because there are often
dependencies. Some of these issues can be mitigated by
dependency_lifetime, which is a good contribution. But I think the use
of leaky_lifetime shouldn't be explicitly discouraged. I think it
would be more appropriate to name the class simply
"unmanaged_lifetime".

4. I think multiton is a good idea, but its execution is incomplete.
If it's in the library, my feeling is that it should implement the
requirements of the standard Pair Associative Container fully. (I'd
also suggest calling it singleton_map).

5. The polymorphic_pointer and polymorphic_reference helpers seem
sketchy to me somehow. I don't understand their use. I'd like to see
use cases in the documentation.

6. The exists(), is_null() and operator bool() methods fall prey to a
well known C++ pitfall. Have a look at Bjorn Karlsson's article, "The
Safe Bool Idiom": http://www.artima.com/cppsource/safebool.html.

7. I want to reiterate my overwhelming disagreement with both the
necessity for, and the approach taken, by the pure virtual
enable_singleton_creation method. This has been discussed at length in
the thread rooted at
http://aspn.activestate.com/ASPN/Mail/Message/2531666.

8. The docs are sorely lacking in detail. For example, the methods in
the basic_singleton and singleton_ex interfaces are not enumerated
anywhere, and none of the examples I saw show how to get an instance
of a singleton. All that I felt was illustrated were various
regurgitations of the "curiously recurring template pattern."
Personally, this gives me the impression (justified or not) that Jason
has been focusing more on the cleverness of his implementation than
what it has to offer in terms of genuine novelty or general
usefulness. Which brings me to my last point:

9. When implementing a Tree library has been discussed on the Boost
mailing list, the biggest point that I feel was made in that
discussion was (paraphrasing) "any idiot can implement a tree, but
thinking about the discrete concepts that comprise the wide variety of
trees, and about the concepts' interoperability with other existing
concepts such as standard containers and algorithms, is a much harder
task." I feel the same critique applies here. The library does offer a
novel concept embodied by "multiton", but my comments about multiton's
failure to implement Pair Associative Container and of the library's
use of a creator policy rather than standard allocators, are examples
of this. I feel that some deeper thought should be given to the
concepts: all the rest is fluff.

To address the standard questions, briefly:

    * What is your evaluation of the design?
A good basic idea with some excellent, novel contributions, but some
design problems (primarily inattention to concepts)

    * What is your evaluation of the implementation?
Problematic (see above)

    * What is your evaluation of the documentation?
Very incomplete. Interfaces are not completely described, some
implementation detail remains in the interfaces, and use cases are
lacking.

    * What is your evaluation of the potential usefulness of the library?
Potentially very useful. I look forward to seeing some implementation
of the singleton pattern make its way into Boost eventually.

    * Did you try to use the library?  With what compiler?  Did you
have any problems?
I did not use it.

    * How much effort did you put into your evaluation? A glance? A
quick reading? In-depth study?
Somewhere between a quick reading and an in-depth study. I focused on
some areas and skimmed others.

    * Are you knowledgeable about the problem domain?
Yes, reasonably knowledgeable. Just like the rest of us I've
implemented enough singletons and "multitons" to know about the basics
of design, and the relevant issues in production code.

Again, to summarize, I think Jason's work shows promise, but needs a
lot of work before I'd consider it acceptable.

Regards,
dr

On 5/4/05, Pavel Vozenilek <pavel_vozenilek_at_[hidden]> wrote:
> The formal review of Singleton library written by Jason Hise
> starts today, May 5th, and lasts 10 days.