Boost logo

Boost :

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.


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk