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

From: Gennadiy Rozental (rogeeff_at_[hidden])
Date: 2008-01-15 21:44:17

"John Torjo" <john.groups_at_[hidden]> wrote in message 
news:478B978A.6090503_at_torjo.com...
> Hi all,

> * What is your evaluation of the design?

In short: in places too complicated, in places plain wrong.

In details:

1. mutexed_singleton

This should be singleton<synchronized<T> >. Current design mixes two 
orthogonal concepts: uniqueness of class instance and class methods 
synchronization and this is obviously wrong.

2. thread_specific_singleton.

The same as above. This should be singleton<thread_specific<T> >. Current 
design mixes two orthogonal concepts: uniqueness of class instance and 
thread specific object storage.

3. public construction and boost::restricted

I don't like this solution and very much prefer private constructor with 
friend declaration (especially if encapsulated into single macro). This will 
be a never ending source of confusion (what is boost::restricted, can I have 
restricted access, how do I create restricted instances etc) and doesn't 
really protect - once can still create instances of my class with valid C++ 
code.

4. DLL support

Explanation of DLL issues in docs is very unclear. No examples provided. No 
tests showing that solution actually works. Accordingly I can't really 
comment on it, but from what I got I really doubt it.

5. instance proxy.

Given complete absence of rationale of this design, I can't really devise 
why is was chosen, but it seems wrong. We end up with pricy complication 
with unclear purposes. What's wrong with just returning T& from instance 
method? The same time we can get rid of all this member_dereference staff 
and it's limitations.

6. lease.

Pretty much the same as above. synchronized<T> should have it's own lock 
facility. Otherwise instance() should be straightforward access to the 
instance and no need for any proxies/leases.

7. MT support

Setting aside mutexed and thread specific singletons covered above, regular 
singleton handling of MT is wrong as well. In simple words my primary 
complain: you can't use define based solution for switching MT/non MT. It 
should be template parameter. As of now there is no singleton that can work 
in presence of threads but without disregard to them.

8. No Meyer's singleton

What if do not want all this manager staff?

> * What is your evaluation of the implementation?
> * What is your evaluation of the documentation?

As usual these days, scarce to the point of being useless. It also 
incorrect - it doesn't cover third template parameter in reference for 
example. Singleton is very basic and quite well known design pattern The 
documentation should explain in details all design choices, compare with 
different solutions and explain advantages. There are number of existing 
pitfalls with singleton pattern - all have to be covered, starting with 
double locking idiom problem and they it's solved (if they are).

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

Shaky. We'll either keep using trivial Meyer's-like singleton or need 
solution which actually make sense and solve real problems. In later case it 
still need require big disclaimer somewhere on top: do not overuse.

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

No.

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

study

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

No.

Gennadiy