C++ push_back into array changes object's variables

Question

I am new to C++, and am doing a physics project with it. There is a problem that I've run into: I have made a Photon class that has a member var called energy (float). In a loop (which parses a txt file of values), when ever I create a new Photon, I assign it a certain energy (as a string - which I later convert to float) through a constructor. Then I push the photon into the a Photon vector. When I cout the energies right after Photon creation, they are as expected, and correspond correctly to txt file.

But, immediately after the loop when I loop again through the vector of Photons, and print photons[i].energy, the values are weirdly changed - some of them even NaN!

void Event::addPhoton(string data) {
    Photon p(data); // the string data is parsed in constructor
    cout << "the energy: " << p.energy << " \n"; // energies output here are correct
    photons.push_back(p);

    // BUG somewhere here
    cout << "Update: Just added a photon. Current photon energies: ***" << endl;
    for (int i = 0; i < photons.size(); i++) {
        cout << photons[i].energy << ", "; // here, values have changed !!
    }
    cout << endl;
}

Any ideas?

Update - Photon class:

#include <stdlib.h>

#include "Photon.h"

Photon::Photon() {

}

Photon::Photon(const Photon& orig) {
}

Photon::~Photon() {
}

Photon::Photon(string data) {
    setData(data);
    std::cout << "Set photon energy to " << energy << std::endl;
}

//parse momentum, energy values .. from data string
void Photon::setData(string data) {
    std::cout << "datastring" << data << std::endl;
    string delimiter = " ";
    size_t pos = 0;
    string token;
    vector<string> values;

    size_t numDels = std::count(data.begin(), data.end(), ' ');

    ///while ((pos = data.find(delimiter)) != string::npos ) {
    for (int i = 0; i < numDels+1; i++) {
        pos = data.find(delimiter);
        token = data.substr(0, pos);
        values.push_back(token);
        data.erase(0, pos + delimiter.length());
    }

    momentum = Momentum(atof(values[0].c_str()), atof(values[1].c_str()), atof(values[2].c_str()));

    energy = atof(values[3].c_str());
}

float Photon::getEnergy() {
    return energy;
}

Answer 1

push_back makes use of Photon's copy constructor, at the moment it appears that does nothing so it would result in a Photon that has randomly initialized values.

If you're on a compiler that supports C++11 you can skip implementing one by just defining it as

Photon(const Photon&) = default;

in your header, this tells the compiler that you want a full copy of the entire object. If you don't define one the compiler will generate one for you. There are readability reasons however that would encourage using defaulting however.

In this particular case if your compiler supports C++11 you can always just use emplace_back and construct the object in place in the vector.

photons.emplace_back(data);

Answer 2

Short

You just want to remove the copy-constructor of your class Photon::Photon(const Photon& orig) {} and it should work.

Note: You can also remove the destructor since you probably don't need it.

Note2: A default copy constructor (which will be auto-generated if you remove yours) requires the members of Photon to either be trivially copyable or to have a copy constructor on their own.

The problem

In fact your Photon copy constructor is empty. Therefore every fundamental-type value or POD is default initialized (with random values).

The standard says:

C++11, [class.base.init], 12.6.2/8

In a non-delegating constructor, if a given non-static data member is not designated by a mem-initializer-id (including the case where there is no mem-initializer-list because the constructor has no ctor-initializer) and the entity is not a virtual base class of an abstract class (10.4), then:

[...]

• otherwise, the entity is default-initialized.

What means default initialized here?

C++11, 8.5/11

If no initializer is specified for an object, the object is default-initialized; if no initialization is performed, an object with automatic or dynamic storage duration has indeterminate value.

That means if you do not provide a member-initializer-list for your (copy)constructor and if you do not assign meaningful values in the (copy)constructor, you'll end up having garbage in your values after the (copy)constructor is done.

So in the line photons.push_back(p); you copy p into the vector where you'll end up having a garbage-initialized entry.

Solution

So my advice is a follows: Strip the following functions and design a default constructor providing meaningful values for the members.

Remove:

Photon::Photon(const Photon& orig) {}
Photon::~Photon() {}

Add:

Photo::Photon (void) : 
  momentum(/*any value required by Momentum constructor*/), 
  energy(0.0) 
{
}

You don't need the destructor since you don't manage resources. Therefore, you also do not need to provide a copy constructor since the compiler will simply provide one for you that will copy each member of your Photon class.

Your example using push_back should work then but you can also use emplace_back if you use a C++11 compiler since the will just "hand-over" its parameters to a suitable constructor.

Instead of

Photon p(data); // the string data is parsed in constructor
photons.push_back(p);

you can also just

photons.emplace_back(data); 

You won't make any copies here since the new object in the vector will be constructed using the data string in place.

Note on defaulting the copy constructor in C++11

Photon(const Photon&) = default;

The explicit defaulting of the copy constructor is not more readable in my opinion but you may want to explicitly default it if you do not want it to be public but protected (or even private).

Implicit constructors, destructors and assignment operators are inline public members of their class by default.

Answer 3

When you insert an object into a vector, a copy of the object is (normally) made using the object's copy constructor. Your copy constructor does nothing, so the energy value of the copied photon is left uninitialized. To fix this, make the copy constructor of Photon copy over the energy value from the original photon.

Photon::Photon(const Photon& orig): energy(orig.energy) {
    // etc.
}

Answer 4

You need to give us the code, that produces the bug.

By the way the energy member should be float. If your input source is a text file, the energy values should be parsed to a floating point number (float or double) before being passed to the Photon constructor.

If you just want to achieve a memberwise copying you can even omit the copy constructor. The C++ compiler will generate one for you.