Why Static data member in class updates not correctly when sending it to a function?

Question

After execution Goomba::liveGoombas is equal to some minus value. I debuged it but did not understand why it launches destructor more times that constructor. Why here it is working not correctly?

// Here is a simple Goomba class. It just keeps track of how many Goombas are alive.

class Goomba
{
public:
  static int liveGoombas;

  Goomba() { liveGoombas++; }
  ~Goomba() { liveGoombas--; }
};

int Goomba::liveGoombas = 0;

// And a Goomba legion class. Please don't change this class.
class GoombaLegion
{
public:
  void add(Goomba goomba)
  {
    goombas.push_back(goomba); //it seems that something wrong in this function
  }

private:
  std::vector<Goomba> goombas;
};

void goombas()
{
  {
    GoombaLegion legion;
  }

  // The legion went out of scope and was destroyed. But how many Goombas are alive?
  std::cout << "There are " << Goomba::liveGoombas << " live goombas" << std::endl;
}



int main()
{
  goombas();

}

Answer 1

There are other constructors that will be created by the compiler if you do not specify your own implementations. These are the copy constructor and, in the case of C++11 and newer, the move constructor.

When you saw negative counts it can be explained by one of the compiler generated constructors having been used. Hence the the number of instances went up but liveGoombasdid not.

To get an accurate count you should change Goomba to be like below.

If you the using a compiler with move semantics enabled (C++0x/C++11 and newer):

class Goomba
{
public:
  static int liveGoombas;

  Goomba() { liveGoombas++; }
  Goomba(const Goomba&) { liveGoombas++; }
  Goomba(Goomba&&) { liveGoombas++; }

  // Need to explicitly state we want default or it will be deleted
  // due to the above move constructor having been defined.
  Goomba & operator = (const Goomba&) = default;

  // Not really essential but including for completeness 
  Goomba & operator = (const Goomba&&) = default;

  ~Goomba() { liveGoombas--; }
};

Otherwise:

class Goomba
{
public:
  static int liveGoombas;

  Goomba() { liveGoombas++; }
  Goomba(const Goomba&) { liveGoombas++; }

  ~Goomba() { liveGoombas--; }
};

The default assignment operators are fine as they do not create new instances but just modify existing ones. As such the instance count should not be changed by them.

Answer 2

None of the other answers were totally accurate.

First, do not declare a move constructor as taking a const argument, this is pointless, and not idiomatic, as you can at best have the same performance as a copy; moreover, moving implies modification of the 'moved from' object.

Second, having Goomba(Goomba&&) { liveGoombas++; } breaks if you do the following:

Goomba g;
Goomba b(std::move(g));
std::cout << Goomba::liveGoombas << std::endl; // prints 2, is that really correct?

This should do the trick for a single threaded program (live version here):

class Goomba
{
  bool alive;
public:
  static int liveGoombas;

  Goomba()
    : alive(true)
  { liveGoombas++; }

  Goomba(const Goomba& rhs)
    : alive(rhs.alive)
  {
    if (alive)
      liveGoombas++;
  }

  // Move constructor
  Goomba(Goomba&& rhs)
    : alive(std::move(rhs.alive))
  {
    if (alive)
      liveGoombas++;
    rhs.kill(); // modifies moved from object!
  }

  Goomba& operator =(const Goomba& rhs)
  {
    kill();
    alive = rhs.alive;
    if (alive)
      liveGoombas++;
    return *this;
  }

  // Move assignment
  Goomba& operator =(Goomba&& rhs)
  {
    kill();
    alive = std::move(rhs.alive);
    if (alive)
      liveGoombas++;
    rhs.kill(); // modifies moved from object!
    return *this;
  }

  ~Goomba() {
    kill();
  }

private:
  void kill() {
    if (alive) {
      liveGoombas--;
      alive = false;

      // insert other death/dying code here
    }
  }
};

Note: It's very common to consider a moved from object as being "destructed," even though that's not at all the case.

Answer 3

When passing by value Goomba to the function you are silently copying it, moreover you need to copy goombas with the push_back operations and the possible reallocations the vector occasionally does.

In order to see where (in the code) you are implicitly using these functions the default constructed copy and assignment operators, define them as private and with no implementation, the compiler will give you errors referring to the lines where you need those operators.

Probably you want to give an implementation of copy constructor and assignment along the line of the constructor.

C++11

If you define the move constructor:

Goomba(const Goomba&&) { liveGoombas++; }

you better let the compiler generate these two for you:

Goomba& operator=(const Goomba&&) = default; 
Goomba& operator=(const Goomba&) = default; 

otherwise they are automatically deleted.

If you don't define the move constructor the assignment is not deleted but it's still a good idea to explicitly state that the default assignment is good for you:

Goomba& operator=(const Goomba&) = default;