1

I am getting some errors when I try to put declaration and definition of a class into separatte hpp and cpp file. could you help me fix it please. I am trying to manipulate a singleton like this:

sing.hpp:

class GlobalClass {
    int m_value;
    static GlobalClass *s_instance;
    GlobalClass(int);

  public:
    int get_value();
    void set_value(int v);
    static GlobalClass *instance(); };

sing.cpp:

#include"sing.hpp"
GlobalClass::GlobalClass(int v = 0)
{
    this->m_value = v;
}

int GlobalClass::get_value()
{
    return this->m_value;
}

void GlobalClass::set_value(int v)
{
    this->m_value = v;
}

static GlobalClass GlobalClass::*instance()
{
    if (!s_instance)
        s_instance = new GlobalClass;
    return s_instance;
}

main.cpp:

#include "sing.hpp"
int main()
{
    GlobalClass *s=0;
}

command and errors are:

~/workspace/singleton$ g++  main.cpp sing.cpp 
sing.cpp: In function ‘GlobalClass GlobalClass::* instance()’:
sing.cpp:19:10: error: ‘s_instance’ was not declared in this scope
sing.cpp:2:1: error: ‘GlobalClass::GlobalClass(int)’ is private
sing.cpp:20:23: error: within this context
sing.cpp:21:12: error: ‘s_instance’ was not declared in this scope
Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
rahman
  • 4,820
  • 16
  • 52
  • 86
  • Once you get your singleton working, you may want to consider whether it's [a good idea](http://stackoverflow.com/questions/137975). (Although that question doesn't mention the particular difficulties of managing them in C++, such as the memory leak and non-thread-safety of your approach.) – Mike Seymour Mar 07 '12 at 11:57

4 Answers4

4
static GlobalClass GlobalClass::*instance()
{
    if (!s_instance)
        s_instance = new GlobalClass;
    return s_instance;
}

This definition shouldn't have the static tag on it. Only the declaration.

As it is, you're not actually defining a member function; if you provided a s_instance variable you'd then get errors about that.

Also the * is in the wrong place.

You'll also later get link errors about s_instance, since you didn't define it.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
3

Your definition of instance has two errors:

  1. The static qualifier is erroneous.
  2. The syntax for the return type got scrambled. The pointer belongs to the type, not the name of the function:

    GlobalClass* GlobalClass::instance()
    {
        if (!s_instance)
            s_instance = new GlobalClass;
        return s_instance;
    }
    

Furthermore, you also need to define the static member s_instance as others have noted.

GlobalClass* GlobalClass::s_instance = 0;

But this code has another problem: it leaks memory. Don’t use raw pointers.

Finally, this code isn’t thread safe and this may in some situtaions be a huge problem. Assuming that you can guarantee that your code is never going to run in multi-threaded scenarios, go ahead. Otherwise, you probably want to change it (and who can offer such a strong guarantee anyway?).

Konrad Rudolph
  • 530,221
  • 131
  • 937
  • 1,214
  • "redundant" and "erroneous" are mutually exclusive in my view; the former implies non-necessity but also no harm – Lightness Races in Orbit Mar 07 '12 at 10:54
  • I don't totally agree about the raw pointers. Why make your application spend 2 minutes shutting down whilst tearing down its singletons when you have the perfect garbage collector available - the fact that a process releases all its resources when it terminates. – CashCow Mar 07 '12 at 10:55
  • 1
    @CashCow A process does most emphatically *not* release all its resources when it terminates. It’s true that most (?) modern operating systems take care of some common resources. But by no means of all resources. There is always the odd handle that you have to release yourself. The problem is: once you don’t call *one* object’s destructor, the clean-up chain is interrupted and such a resource may not get released at all. This must never happen, and there is no excuse to let it happen. Always make sure all destructors are called. This is the only way to ensure that all resources are released. – Konrad Rudolph Mar 07 '12 at 10:58
  • @CashCow: `the fact that a process releases all its resources when it terminates.` How is that a "fact"? – Lightness Races in Orbit Mar 07 '12 at 11:05
  • ok, reword that. The operating system will reclaim the resources. It is still automatic garbage collection for you when you want it. And when I shut down any application it's because I don't want it running anymore. It annoys me when firefox or Visual Studio or any other application continues running for a period after I have shut it down. – CashCow Mar 07 '12 at 14:38
1

Static identifiers must be defined as well as declared.

So, put s_instance in your sing.cpp. And I believe you should initialise it to NULL.

Mr Lister
  • 45,515
  • 15
  • 108
  • 150
1

In sing.cpp you need to instantiate s_instance like this:

GlobalClass * GlobalClass::s_instance = NULL;

And your function static GlobalClass GlobalClass::*instance() in the cpp file shouldn't have the static keyword.

Charles Beattie
  • 5,739
  • 1
  • 29
  • 32