Re: SingletonWithManager

From: rogeeff (rogeeff_at_[hidden])
Date: 2001-12-09 04:10:24

--- In boost_at_y..., peter.nordlund_at_c... wrote:
> Hi Gennadiy, Mattias and others,
> 
> I have tried to address some of Gennadiy's suggestions.
> The ones I think that I have solved, I have mentioned 
> in Gennadiy's text below.
> I have uploaded a zip-file in the directory SingletonWithManager.
> 
> If someone is interested in my suggested solution I will put
> more work into it.
> 
> Best regards,
> Peter
> 

Hi, Peter

I finally found a time to look through your code. Several general 
comments:
1. a code your are submitting contain a lot of 'noise'. Executables, 
log files, make files for gcc compiler, scripts, temporary files, 
copy of some boost files. The files are not structured: some test 
files appears in test directory, some in a root directory. 
2. You seem to be developing using gcc compiler, which if reflected 
in a lot of places.
3. You are using partial specialization - it's not portable.
4. Documentation is incomplete and does not cover most of the aspect 
of your design decisions.

In details:

managed-singleton.hpp:

> #  define NAMESPACE_BUG //GCC gets an ICE ...

Did you try to use boost config system?

> #  define TEST1_MGR_T Test1::ManagerType

What is this all about? Why does it belong to main generic header?

> #define NONCOPYABLE :public noncopyable

Using of this define seems to be more obscure than the text you 
substituting.

> template <int D_PHASE = 1 /* Destruction phase */ >

Use readable names instead of commenting. And another general 
comment: better stick to one naming convention (boost one for 
example) and use clear naming. You are using different formats and 
names you are using are obscure and misleading: typename MANAGER, int 
D_PHASE, class Singleton, static T* theInstance_, struct 
NoCleanupMgr2, struct NoCleanupMgr2NT, class ExtendedDM, struct 
deleteInstMgr, SingletonCollElementType, static void Register(...), 
static void destruct()

> template <typename T /* Singleton type */, 
>            typename MANAGER> 
> class Singleton NONCOPYABLE { 

Here I am *completely* disagree with you. Here three main points:

* The whole idea is with reusable design to factor out orthogonal 
policies so that we will be able to choose a behavior just by using a 
proper policy in an appropriate axis. If I need to introduce some 
specific feature I could create a variation of one of existent 
policies in one axis and reuse an existent in others. With this 
design I will end up rewriting whole manager every time I need to 
introduce small variation. Why would I need a generic class then?
* This design forced you to introduce partial specialization, caused 
every common 'feature' of Manager class will need to have one more 
(one is T) template parameter - MANAGER as a key to select a concrete 
Manager class. Example: your instance management feature.
* With this design the Singleton class by itself does not bring any 
value other that fixed interface (which could be valuable but in that 
case, when interface consist from only one function instance, it does 
not)

> class Singleton { 
public:
> 
>   inline static T& instance();            //>x
>   inline static const T& constInstance(); //>x

Three comments:

* inline could be omitted
* what this comment all over your code mean?
* I disagee with design that has 2 access methods to instance. First 
of all I should be able to keep only one by using some kind of a 
traits class. And second I want to be able to have any return type 
for several reasons:
  a. I want efficient readwrite access - use T&
  b. I want efficient readonly access - use T const&
  c. My usage pattern assume keeping undefined references on type T - 
use T*
  d. but access should be readonly - use T const* 

nocleanup-manager.hpp:
nocleanup-manager-no-member-templ.hpp
cleanup-manager.hpp

Already covered above.

select1st.hpp

This will be a part of boost soon.

utility.hpp

Why not to use boost version?

io.hpp

It's another example of 'noise'. You are presenting 8kb file with 
some your specific code just to show an example of some technique. 
This should in example section and tiny with obvious design and 
implementation 

destruction_manager.hpp

> template <typename T, typename MANAGER> struct InstanceTypeTraits 

Now you will need a partial specialization every time to change 
traits.

>  class BadDependencies {};

Would it be better to inherit from std::logic_error

>  typedef std::pair ...; 

You never included <utility>

>  typedef std::pair<int, InstanceManagerBase*> 
SingletonCollElementType;
>  typedef std::list<SingletonCollElementType>  
SingletonCollectionType;

>  static void Register(SingletonCollectionType& newSingleton) throw
(); // NOTHROW

* Before you used comment // NOTHROW but did not put throw()
* In most cases you will use this function with 1 element list. Why 
do not create a convenient interface that will accept 
SingletonCollElementType also?

>  static void destruct() throw(DestructionOngoing);    //>x

This is one of couple places where you declare an exception list.
You need to be consistent.

>  static AutoDestructor autoDestructor_;

AutoDestructor is an implementation detail. Move it to implementation 
file completely.

>  template<typename T, typename MANAGER> friend class Singleton;

are you sure it's portable?

>  enum { DestructionPhase = D_PHASE };

declared but never used, though comment promised to order destruction 
by it.

 
> class DestructionManagerBase::AutoDestructor NONCOPYABLE {
> public:
>  ~AutoDestructor() throw(DestructionOngoing) { destruct(); }
>  //private:
>  //AutoDestructor(const AutoDestructor&);            // Shouldn't > 
be used
>  //AutoDestructor& operator=(const AutoDestructor&); // Shouldn't 
be used
> };

What method destruct() are you calling. It's obscure. class declared 
noncopyable no need for additional comments.

> template <typename EXCEPTION, typename T = void> 
> class Lock NONCOPYABLE {
> ...
> }
> and later
>  // Failing to acquire this lock means that a circular call to
>  // instantiate() is ongoing.
>  Lock<CircularInstantiation, T> throwOnCircularInstantiation;  //>x
>  // Failing to acquire this lock means that destruction is ongoing.
>  try { Lock<DestructionOngoing> throwOnDestructionOngoing; } 
>  catch(...) { throw InstantiationFailedDestructionOngoing(); }  //>x

usage of this class very unclear and is not documented. But in any 
case it does not seem to be a part of generic template it should be a 
separate policy.

>  // Here we could acquire a lock if we want to use
>  // "Double-Checked Locking" for thread-safe creation of Singleton.

Double-Checked Locking is not thread safe. See in this forum 
reference somewhere.

>   SingletonCollectionType 
>    preAlloc(1, SingletonCollElementType(D_PHASE, 0));        //>x0
>  preAlloc.back().second = new InstanceManager<T, SelfType_>; //>x1
>  BaseType::Register(preAlloc);

* Aren't you supposed to use DestructionPhase here?
* you would better introduce an interface that does not require 
creating a fake list.

extended-dm.hpp:

I did not get what is your extension is so: no description - no 
comment.

destruction-manager.cpp

>  assert(!singletons_.empty());

You never included an <cassert>

>  } catch(...) { std::terminate(); }

what and why do you want to catch?

lokiadapt.cpp:

I did not get a point to adapt a Loki Singleton. It seems to work by 
itself.

inst-before-main.cpp:

> static Instantiator i;

If I need an instantiator, why would I need all this machinery?

That is all.

Regards,

Gennadiy.