question re. usage of "static" within static member functions of aclass

S

ssb

Hi All,
During a code review, I found the following lines of code:

Class Data:public MyBaseClass
{
Q_OBJECT

Public:
static Data* instance();
~Data();
….
….
….


// NOTE: no static member variables

private:
Data();

}


The "instance" method was implemented as follows:

Data* Data::instance()
{
static Data* model = new Data();
return model;
}


I have never come across a situation where a pointer was set to static
in such a case. Is this valid?What are the potential pitfalls in such
programming practices?

Thanks and Best regards
~ssb
 
C

Chris M. Thomasson

ssb said:
Hi All,
During a code review, I found the following lines of code:
[...]

The "instance" method was implemented as follows:
Data* Data::instance()
{
static Data* model = new Data();
return model;
}

I have never come across a situation where a pointer was set to static
in such a case. Is this valid?

It's a singleton.



What are the potential pitfalls in such programming practices?

The storage that `model' points to will never be destroyed, also it's not
thread-safe.
 
S

Shrikumar

Thanks for the quick reply, Chris.
I was wondering about the static pointer part - I have always seen
static variables (that are not pointers) in use, but never a static
pointer (even if it is to guarantee that the singleton always returns
the *same* instance of the Class). Is a static pointer (as in the
instance function) a perfectly valid use of the "static" keyword?

Hi All,
During a code review, I found the following lines of code:
[...]

The "instance" method was implemented as follows:
Data* Data::instance()
{
     static Data* model = new Data();
     return model;
}
I have never come across a situation where a pointer was set to static
in such a case. Is this valid?

It's a singleton.
What are the potential pitfalls in such programming practices?

The storage that `model' points to will never be destroyed, also it's not
thread-safe.
 
Joined
Sep 7, 2009
Messages
2
Reaction score
0
i didnt Understand the exact attempt your making here to acheive(Singleton? Finalising Derived Class?).

However Code is not Correct in either case and it is Causing Memory leak.

Each and every time when you call Data::instance() Function it creates new Object and Overwrites Static variable with New Object Address.
 
F

Francesco

Hi All,
During a code review, I found the following lines of code:
The "instance" method was implemented as follows:
Data* Data::instance()
{
static Data* model = new Data();
return model;
}
I have never come across a situation where a pointer was set to static
in such a case. Is this valid?
It's a singleton.
The storage that `model' points to will never be destroyed, also it's not
thread-safe.

Thanks for the quick reply, Chris.
I was wondering about the static pointer part - I have always seen
static variables (that are not pointers) in use, but never a static
pointer (even if it is to guarantee that the singleton always returns
the *same* instance of the Class). Is a static pointer (as in the
instance function) a perfectly valid use of the "static" keyword?

I think it is. After all, that pointer should be initialized only once
and operator new should be called only once - the first time the
method is invoked - but this confirmation should be implied into
Chris' sentence: "It's a singleton".

As usual, there are many ways to achieve the same target while
programming.

But was that coder really meant to implement it that way?

The implementation you reviewed:
-------
Data* Data::instance()
{
static Data* model = new Data();
return model;
}
-------

Gives the same result[1] of:
-------
Data* Data::instance() {
static Data model;
return &model;
}
-------

[1] Well, more or less "the same result". My mod could be preferred
because it doesn't use dynamic memory, but in order to avoid some
client to see the pointer as something that could/should be deleted
sooner or later, the code could return the object as a reference.

Changing the method declaration, it could be implemented it in this
way:
-------
Data& Data::instance() {
static Data model;
return model;
}
-------

or also as:

-------
// data.cpp
#include "data.h"

namespace {
Data model;
}

Data& Data::instance() {
return &model;
}
-------

Which doesn't use the "static" keyword at all.

But of course, being a code-reviewer, you should already know all the
above.

Let's say I'm taking the chance to see if _I_ got it right, showing my
code for review here in clc++ ;-)

Cheers,
Francesco

PS: please do not top-post the next time, moreover because it messes
up subsequent replies to your top-posting.
 
F

Francesco

-------
// data.cpp
#include "data.h"

namespace {
    Data model;

}

Data& Data::instance() {
     return &model;}

-------

Ops, kill that & before model!
 
P

Pavel

Shrikumar said:
Thanks for the quick reply, Chris.
I was wondering about the static pointer part - I have always seen
static variables (that are not pointers) in use, but never a static
pointer (even if it is to guarantee that the singleton always returns
the *same* instance of the Class). Is a static pointer (as in the
instance function) a perfectly valid use of the "static" keyword?
It is valid to declare pointers static if that's what you mean. On a
side note, I think they could avoid both using pointer and the memory
leak (which may be harmless in this case though) as follows:

{
static Data model;
return &model;
}


Hope this will help,
Pavel
Hi All,
During a code review, I found the following lines of code: [...]

The "instance" method was implemented as follows:
Data* Data::instance()
{
static Data* model = new Data();
return model;
}
I have never come across a situation where a pointer was set to static
in such a case. Is this valid?
It's a singleton.
What are the potential pitfalls in such programming practices?
The storage that `model' points to will never be destroyed, also it's not
thread-safe.
 
F

Francesco

Ops, kill that & before model!

Heck, no, if that's a singleton the ctor is private, hence the module
cannot instantiate it. Sorry, kill all the above.

Francesco
 
F

Francesco

This brings along the destruction problems at the end of the program. The
singleton might be destroyed too early this way, when some code still
might need access to it. When created dynamically, this problem does not
occur, and the memory is reclaimed by the OS upon process exit anyway, so
there is no memory leak anyway. The singleton destructor is not run in
this case, so one should not put something essential there.

Ahhhrgh! Thanks a lot for pointing this out - I was just stomping on
the same problem with my suggestion.

So then, if I got your post right Paavo: in order to circumvent the
destruction-order problem I should create the singleton instance as a
dynamic object _and_ I should not free it in the destructor -
otherwise I would be throwing in the destruction-order problem again.

Side question - once I'm there - is the following fine?

-------
Data& Data::instance() {
static Data* model = new Data();
return *model;
}
-------

I hope so =/

Best regards,
Francesco
 
F

Francesco

Francesco <[email protected]> kirjutas:








Yes I think this is fine, my singletons typically look alike.

Good to know, I suppose the following should be fine too:

-------
Data& Data::instance() {
static Data& model = *(new Data());
// or also, simply:
// static Data& model = *new Data;
return model;
}
-------

I recall someone who wrote "programming _IS_ decision making" ;-)
In addition, in case of multithreaded applications I usually call all
such instance() methods in the beginning of the program (or in the init
routine of a dynamically loaded library), in order to avoid potential
thread clash in the singleton creation. One could attempt to protect the
singleton creation by a static mutex, but then one would be back at the
statics destruction order problems.

Thank you for the further details about multi-threading, I'm adding
them to my "toolbox", along with all the good new things I'm learning
here on clc++.

Cheers,
Francesco
 
J

James Kanze

During a code review, I found the following lines of code:
The "instance" method was implemented as follows:
Data* Data::instance()
{
static Data* model = new Data();
return model;
}
I have never come across a situation where a pointer was set
to static in such a case. Is this valid?
It's a singleton.

And to answer the question, it's perfectly valid. A pointer is
an object, just like any other variable, and obeys the same
rules as other variables.
The storage that `model' points to will never be destroyed,
also it's not thread-safe.

Not being destroyed is presumably the reason the code is written
this way. Most of the time, you don't want a singleton to be
destructed. In other word, it's a feature designed to avoid
pitfalls. As for thread-safety, it depends on the
implementation, it is thread safe---modulo bugs---in g++. (But
you're probably right for most implementations.)
 
J

James Kanze

During a code review, I found the following lines of
code:
[...]
The "instance" method was implemented as follows:
Data* Data::instance()
{
static Data* model = new Data();
return model;
}
I have never come across a situation where a pointer was
set to static in such a case. Is this valid?
It's a singleton.
What are the potential pitfalls in such programming
practices?
The storage that `model' points to will never be
destroyed, also it's not thread-safe.
I was wondering about the static pointer part - I have
always seen static variables (that are not pointers) in use,
but never a static pointer (even if it is to guarantee that
the singleton always returns the *same* instance of the
Class). Is a static pointer (as in the instance function) a
perfectly valid use of the "static" keyword?

A pointer is an object, just like any other type, and a variable
of pointer type follows exactly the same rules as a variable of
any other type.
I think it is. After all, that pointer should be initialized
only once and operator new should be called only once - the
first time the method is invoked - but this confirmation
should be implied into Chris' sentence: "It's a singleton".

Maybe. The fundamental definition of a singleton is that the
class can only have one instance. The example just happens to
be one of the more frequent implementation techniques.
As usual, there are many ways to achieve the same target while
programming.

And many ways to achieve something that is almost the same
thing, but subtly different.
But was that coder really meant to implement it that way?
The implementation you reviewed:
-------
Data* Data::instance()
{
static Data* model = new Data();
return model;
}

Gives the same result[1] of:
-------
Data* Data::instance() {
static Data model;
return &model;
}
-------

Not a all. In your version, the singleton will be destructed.
Possibly too early. In his version, the singleton will never be
destructed. In most cases, his version is to be preferred.
[1] Well, more or less "the same result". My mod could be
preferred because it doesn't use dynamic memory, but in order
to avoid some client to see the pointer as something that
could/should be deleted sooner or later, the code could return
the object as a reference.

That's an orthogonal issue, and typically, the instance function
of a singleton does return a reference, regardless of the
implementation behind it. That, of course, doesn't prevent the
client from deleting it---often, the destructor will also be
made private as well. But typically, a reference will be used
because the client code isn't expected to delete it, and the
function can never return a null pointer.
Changing the method declaration, it could be implemented it in this
way:
-------
Data& Data::instance() {
static Data model;
return model;
}

or also as:
namespace {
Data model;
}
Data& Data::instance() {
return &model;
}
-------
Which doesn't use the "static" keyword at all.

And doesn't avoid the order of initialization issues the other
versions were designed to avoid.
But of course, being a code-reviewer, you should already know
all the above.
Let's say I'm taking the chance to see if _I_ got it right,
showing my code for review here in clc++ ;-)

Not knowing the requirements, it's hard to say. My usual
implementation is:

class Data
{
private:
static Data* ourInstance ;
Data() {}
~Data() {}

public:
static Data& instance() ;
// ...
} ;

Data* Data:: ourInstance = &instance() ;

Data&
Data::instance()
{
if ( ourInstance == NULL ) {
ourInstance = new Data ;
}
return *ourInstance ;
}

This solves the threading issues (for the most part), and avoids
any order of destruction issues, by not destructing the object.
 
J

Joshua Maurice

Good to know, I suppose the following should be fine too:

-------
Data& Data::instance() {
     static Data& model = *(new Data());
     // or also, simply:
     // static Data& model = *new Data;
     return model;}

-------

Someone know offhand the rules for extending the life of a temporary
which is bound to a static reference? Would the temporary here die as
soon as the first function invocation ends? Will it be destroyed
during static de-initialization? I'd instead suggest:

Data& Data::instance()
{ static Data* model = new Data();
return *model;
}
 
J

Joshua Maurice

Someone know offhand the rules for extending the life of a temporary
which is bound to a static reference? Would the temporary here die as
soon as the first function invocation ends? Will it be destroyed
during static de-initialization? I'd instead suggest:

Data& Data::instance()
{   static Data* model = new Data();
    return *model;

}

Ack, hit submit early. I'd suggest that \with\ the caveat "Insert
extra code to make concurrent calls thread-safe if needed", generally
by doing
namespace { bool force_init = (Data::instance(), true); }
which works in most cases (aka where there are no non-trivial threads
before main, or before dlopen or equivalent if this is in the static
init of a dll).
 
C

Chris M. Thomasson

[...]
In addition, in case of multithreaded applications I usually call all
such instance() methods in the beginning of the program (or in the init
routine of a dynamically loaded library), in order to avoid potential
thread clash in the singleton creation. One could attempt to protect the
singleton creation by a static mutex, but then one would be back at the
statics destruction order problems.

Perhaps I am misunderstanding you, but FWIW in POSIX, it's perfectly legal
to create a `pthread_mutex_t' in static storage. There is absolutely no
static's destruction order problems wrt to the mutex itself:
______________________________________________________________________
#include <pthread.h>
#include <exception>
#include <cassert>
#include <cstdio>




#if ! defined (PTHREAD_UNEXPECTED)
# define PTHREAD_UNEXPECTED(s) \
assert(! (s)), std::unexpected()
#endif




class lock_guard
{
pthread_mutex_t& m_mutex;


public:
lock_guard(pthread_mutex_t& mutex) throw()
: m_mutex(mutex)
{
int status = pthread_mutex_lock(&m_mutex);
if (status)
{
PTHREAD_UNEXPECTED(status);
}
}


~lock_guard() throw()
{
int status = pthread_mutex_unlock(&m_mutex);
if (status)
{
PTHREAD_UNEXPECTED(status);
}
}
};




template<typename T>
struct non_destroying_singleton
{
static T& instance()
{
static T* g_instance = NULL;
static pthread_mutex_t g_mutex = PTHREAD_MUTEX_INITIALIZER;

lock_guard guard(g_mutex);

if (! g_instance)
{
g_instance = new T();
}

return *g_instance;
}
};




struct foo
{
foo()
{
std::printf("(%p)->foo::foo()\n", (void*)this);
}


~foo()
{
std::printf("(%p)->foo::~foo()\n", (void*)this);
}
};




int
main()
{
{
foo& f1 = non_destroying_singleton<foo>::instance();
foo& f2 = non_destroying_singleton<foo>::instance();
foo& f3 = non_destroying_singleton<foo>::instance();
foo& f4 = non_destroying_singleton<foo>::instance();
foo& f5 = non_destroying_singleton<foo>::instance();
}

return 0;
}
______________________________________________________________________




The singleton template code above `non_destroying_singleton<T>' is 100%
thread-safe.
 
F

Francesco

On Sep 7, 10:24 am, "Chris M. Thomasson" <[email protected]>
wrote:
During a code review, I found the following lines of
code:
[...]
The "instance" method was implemented as follows:
Data* Data::instance()
{
     static Data* model = new Data();
     return model;
}
I have never come across a situation where a pointer was
set to static in such a case. Is this valid?
It's a singleton.
What are the potential pitfalls in such programming
practices?
The storage that `model' points to will never be
destroyed, also it's not thread-safe.
I was wondering about the static pointer part - I have
always seen static variables (that are not pointers) in use,
but never a static pointer (even if it is to guarantee that
the singleton always returns the *same* instance of the
Class). Is a static pointer (as in the instance function) a
perfectly valid use of the "static" keyword?

A pointer is an object, just like any other type, and a variable
of pointer type follows exactly the same rules as a variable of
any other type.
I think it is. After all, that pointer should be initialized
only once and operator new should be called only once - the
first time the method is invoked - but this confirmation
should be implied into Chris' sentence: "It's a singleton".

Maybe.  The fundamental definition of a singleton is that the
class can only have one instance.  The example just happens to
be one of the more frequent implementation techniques.
As usual, there are many ways to achieve the same target while
programming.

And many ways to achieve something that is almost the same
thing, but subtly different.


But was that coder really meant to implement it that way?
The implementation you reviewed:
-------
Data* Data::instance()
{
     static Data* model = new Data();
     return model;
}
-------
Gives the same result[1] of:
-------
Data* Data::instance() {
     static Data model;
     return &model;
}
-------

Not a all.  In your version, the singleton will be destructed.
Possibly too early.  In his version, the singleton will never be
destructed.  In most cases, his version is to be preferred.
[1] Well, more or less "the same result". My mod could be
preferred because it doesn't use dynamic memory, but in order
to avoid some client to see the pointer as something that
could/should be deleted sooner or later, the code could return
the object as a reference.

That's an orthogonal issue, and typically, the instance function
of a singleton does return a reference, regardless of the
implementation behind it.  That, of course, doesn't prevent the
client from deleting it---often, the destructor will also be
made private as well.  But typically, a reference will be used
because the client code isn't expected to delete it, and the
function can never return a null pointer.


Changing the method declaration, it could be implemented it in this
way:
-------
Data& Data::instance() {
     static Data model;
     return model;
}
-------
or also as:
-------
// data.cpp
#include "data.h"
namespace {
    Data model;
}
Data& Data::instance() {
     return &model;
}

And doesn't avoid the order of initialization issues the other
versions were designed to avoid.

Well, to be precise, the last version above isn't even applicable,
because the ctor is private. I've already straightened all the points
above, partly by myself, mostly with Paavo, as you can see from all
the posts between the one you are quoting from me and the one I'm
replying to. I suppose you didn't receive them. Freakin' NNTP.
Not knowing the requirements, it's hard to say.  My usual
implementation is:

    class Data
    {
    private:
        static Data*    ourInstance ;
        Data() {}
        ~Data() {}

    public:
        static Data&    instance() ;
        //  ...
    } ;

     Data* Data::   ourInstance = &instance() ;

     Data&
     Data::instance()
     {
        if ( ourInstance == NULL ) {
            ourInstance = new Data ;
        }
        return *ourInstance ;
    }

This solves the threading issues (for the most part), and avoids
any order of destruction issues, by not destructing the object.

Thank you for showing an example of your implementation, it helps me
confronting the different approaches and getting a better grip on the
subject.

Best regards,
Francesco
 
F

Francesco

What temporary, exactly?

The code you posted above has already been mentioned.
Ack, hit submit early. I'd suggest that \with\ the caveat "Insert
extra code to make concurrent calls thread-safe if needed", generally
by doing
    namespace { bool force_init = (Data::instance(), true); }
which works in most cases (aka where there are no non-trivial threads
before main, or before dlopen or equivalent if this is in the static
init of a dll).

The above isn't clear to me. What's the difference between this:
namespace { bool force_init = (Data::instance(), true); }
and this:
namespace { Data::instance(); }
assuming the important part is calling Data::instance()? Where should
force_init be used?

In any case, thanks for pointing it out, calling Data::instance()
directly in the module should ensure that the singleton gets
instantiated before main() starts, I suppose.

Best regards,
Francesco
 
F

Francesco

The above isn't clear to me. What's the difference between this:
    namespace { bool force_init = (Data::instance(), true); }
and this:
    namespace { Data::instance(); }
assuming the important part is calling Data::instance()? Where should
force_init be used?

Heck, I got it. Freakin' trick, I should have tried my mod before.
Feel free to crunch the above silly error ignoring this post, I'm
getting accustomed.

Cheers,
Francesco
 
R

REH

Not knowing the requirements, it's hard to say.  My usual
implementation is:

    class Data
    {
    private:
        static Data*    ourInstance ;
        Data() {}
        ~Data() {}

    public:
        static Data&    instance() ;
        //  ...
    } ;

     Data* Data::   ourInstance = &instance() ;

     Data&
     Data::instance()
     {
        if ( ourInstance == NULL ) {
            ourInstance = new Data ;
        }
        return *ourInstance ;
    }

This solves the threading issues (for the most part), and avoids
any order of destruction issues, by not destructing the object.

This isn't any more thread-safe than any of the previous versions. In C
++0x, the static version will be thread-safe, but this version still
won't be.

REH
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,755
Messages
2,569,536
Members
45,010
Latest member
MerrillEic

Latest Threads

Top