int variables change value after being pushed back in a Vector (C++)

Question

This is the class using the Time class and where the "magic" happens. the data is taken out from a text file

    while(i<flightsNumber){
        if(ist>>nameArr>>arr>>airline>>fare>>time){
            Flight flight(dep,arr,nameArr,airline,fare,time);
            flightVector.push_back(flight);
            //pre-check
            cout<<flight.getTime()<<endl;
        }
        else
            error("Error: programData.dat contains invalid data");  
        //post-check
        cout<<flightVector[i].getTime()<<endl;
        i++;
    }

and this is my MyTime class

#include "MyTime.h"

MyTime::MyTime()
    :h(0),m(0){
}

MyTime::MyTime(int hh,int mm)
    :h(hh),m(mm){

    if(hh<0 || mm<0 || mm>59)
        error("Time(): invalid construction");
}

void MyTime::setTime(int hh,int mm){
    if(hh<0 || mm<0 || mm>59)
        error("setTime(): invalid time");
    h=hh;
    m=mm;
}

int MyTime::getHour() const{
    return h;
}

int MyTime::getMinute() const{
    return m;
}


istream& operator>>(istream& is,MyTime& time){
    char ch1;
    int hour,minute;
    is>>hour>>ch1>>minute;
    if(is){
        if(ch1==':'){
            time.h = hour;
            time.m = minute;
        }
        else
            is.setstate(ios_base::failbit);
    }
    else
        is.setstate(ios_base::failbit);
    return is;
}

ostream& operator<<(ostream& os,const MyTime& time){
    return os<<time.h<<":"<<time.m;
}

the output is:

1:12

-33686019: -1414812757

How on earth is this possible?

the value of the instance of MyTime change right after the push_back() function is executed.

Please simplify your test-case. Get rid of all the irrelevant stuff (like reading from an input stream, etc.), and cut this right down to the shortest possible code that still shows the problem. — Oliver Charlesworth, May 24 '12 at 16:36
Do you have correct copy-constructors for `Flight` and `MyTime`, being them implicitly or explicitly defined? — K-ballo, May 24 '12 at 16:38
What does your input data look like? Seeing large negative numbers is sometimes indicative of integer overflow. — alanxz, May 24 '12 at 16:52
Can we see the definition of the MyTime class? By any chance, are h and m members int& instead of int? — abarnert, May 24 '12 at 19:09
After waiting 30 minutes and refreshing, I still don't see your MyTime.h anywhere. Flight.h may also help, in case you're doing something like storing a MyTime& instead of a MyTime. — abarnert, May 24 '12 at 20:24

craigmj · Answer 1 · 2012-05-24T16:57:37.453

1

You are using a vector, with a Flight class, so in order for the vector to correctly copy the Flight object, it requires an operator = and a copy constructor. Do you have these in the Flight class:

class Flight {
public:
  Flight(const Flight &copy);
  const Flight &operator=(const Flight &);
  virtual ~Flight(); /* Good practice, esp. when using containers */
  // etc
}

So make sure with the copy constructor and the = operator that you copy your Time values across.

The only other point you could check is the value of i: I'm assuming you've inited it to 0?

I'm going to demonstrate the copy constructor and the operator = on your Time class, just to keep it simple:

class Time {
protected:
  int h,m;
public:
  Time() { h=m=0; }
  Time(int hour, int minute) : h(hour), m(minute) {}
  Time(const Time &rhs) { operator=(rhs); }
  virtual ~Time() {}
  const Time &operator=(const Time &rhs) {
    h = rhs.h;
    m = rhs.m;
    return rhs;
  }
};

So using this code, I could store Time values into a vector<Time>. I can also use = naturally with Time:

Time a(12,0);
Time b = a;
Time c;
c = b
Time d(a);

The virtual destructor means that when the vector deletes the Time instances it holds, it will use a virtual destructor for them. This isn't particularly useful with this example (hence I've kept the destructor empty), but perhaps a derived class would require particular destructor. For instance:

class AtomicTime : public Time {
public:
    AtomicTime() {
      lockNuclearReactor();
    }
    virtual ~AtomicTime() {
      releaseNuclearReactor();
    }
};

Now, if you have a vector of Time classes:

vector<Time> times;

You can happily go:

AtomicTime at;
times.push_back(at);

And you don't have to worry about the nuclear reactor ;-)

edited May 24 '12 at 16:57

answered May 24 '12 at 16:41

craigmj

4,827
2
18
22

no i dont have those, how can I implent them? I never heard of copy constructors and the keyword virtual – ichigo663 May 24 '12 at 16:49
1

If the class only held a couple of ints, there would be no reason whatsoever to define the copy constructor and assignment operator. – juanchopanza May 24 '12 at 17:14
@user1079367: If you've never heard of copy constructors or the keyword virtual, then you should work your way through [a good book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) before you try to program anything interesting on your own. – Benjamin Lindley May 24 '12 at 17:16
after implementing the copy constructor on the MyTime class, it still gives me the same output, i guess i have to implement it in the flight class as well? but how to? – ichigo663 May 24 '12 at 17:39
2

@user1079367 if `MyTimne` only contains a couple of ints, there is no reason to implement the copy constructor and assignment operator yourself. The ones synthesized by the compiler should be fine. – juanchopanza May 24 '12 at 17:46
why it doesn't work then? anyway it doesn't work with the copy constructor as well – ichigo663 May 24 '12 at 17:48
All of this is almost certainly irrelevant, for the reasons juanchopanza suggested. Classes that only hold values or self-managing objects should rely on the compiler-generated copy constructors, assignment operators, and destructors. It's only classes that directly manage something (freestore, files, etc.) that need to implement them (in which case, you should implement them all—google Rule of Three, or its larger C++11 equivalent). – abarnert May 24 '12 at 19:13
yes but this is not working anyway, did anyone come up with a solution? – ichigo663 May 24 '12 at 19:57
I wouldn't expect it to work. What juanchopanza was saying is that doing what this answer suggests amounts to explicitly writing the exact same code the compiler has already written for you, which will have the exact same problems. – abarnert May 24 '12 at 20:33
I think these comments are correct. Every instance where I've needed to implement my own copy constructor and operator = have involved very complex classes with resource handles (per the AtomicTime eg.) So while the `Time` example above was meant as a simple example, it doesn't seem likely you would need these for your situation. – craigmj May 25 '12 at 11:42

craigmj · Answer 2 · 2012-05-25T21:10:53.467

1

Per the comments to the answer above, assuming your Time and Flight classes only contain values or self-managing objects, you should be fine putting instances into a vector. Which would seem to suggest that your vector index i might be at fault. What happens if you change the line:

cout<<flightVector[i].getTime()<<endl;

to:

cout << (flightVector.rbegin())->getTime() << endl;

Also, can you post the code for your Flight class?

edited May 25 '12 at 21:10

answered May 25 '12 at 11:46

craigmj

4,827
2
18
22

It's possible. For example, if he didn't initialize i before the loop, for example, then he'd push_back to create flightVector[0], then try to output flightVector[1173124989]… But then given that we don't have nearly enough of the code to find the problem, almost anything is possible. – abarnert May 25 '12 at 18:09
Agreed, although what puzzles me is that `flightVector[x]` will be atomatically initialized by the `vector`, so it should have 0 times. The error is probably somewhere in the `Flight` class, but that we've not seen. – craigmj May 25 '12 at 20:46

int variables change value after being pushed back in a Vector (C++)

2 Answers2