Conditional jump depends on unitialized value Valgrind
Hi, I am really lost here, I have airport class which should navigate planes, in its list to runways, with method move, theres a method prepare which changes the direction of flight to all planes, always before move is called, move just increments decrement x and y of plane in its list. But after calling two times in row airport->move(), I get screwed and I really dont know wheres the problem. Have I badly initiazed something? Iterator gets invalidated.
Thanks for help.
Valgrind Stacktrace
Conditional jump or move depends on uninitialised value(s)
==26207== at 0x409601: plane::move() (in /home/xnovak11/Downloads/airport/main)
==26207== by 0x401FBD: airport::move() (in /home/xnovak11/Downloads/airport/main)
==26207== by 0x405FE1: io::start(std::istream&, std:stream&, std:stream&) (in /home/xnovak11/Downloads/airport/main)
This is how I add planes into list in airport
Code:
list<plane*> planes;
list<plane*>::const_iterator planeIterator;
list<plane*>::iterator planeIteratorMoj;
bool airport::addPlane(const plane& p)
{
for(planeIteratorMoj= planes.begin(); planeIteratorMoj!= planes.end(); planeIteratorMoj++)
{
if(p.getName()== (*planeIteratorMoj)->getName())
{
return false;
}
}
plane *p1 = new plane(p);
if(planes.empty())
{
planes.push_back(p1);
if(planes.size()==1)
{
planeIterator = planes.begin();
}
}
else
{
planes.push_back(p1);
planeIterator = planes.begin();
}
return true;
}
This is the method where it fails.
When I call it once, no problem, after second call I get instead of normal number in cout<<after move<< s1 i get like 8795456
Dunno why.
Code:
void airport::move()
{
for(planeIteratorMoj = planes.begin(); planeIteratorMoj!= planes.end(); planeIteratorMoj++)
{
plane * p1 = (*planeIteratorMoj);
int s,w;
p1->getPosition(s,w);
cout<<"before move function"<<s<<","<<w<< p1->getDirection()<<endl;
if((*planeIteratorMoj)->move())
{
delete *planeIteratorMoj;
}
plane * p2 = (*planeIteratorMoj);
int s1,w1;
p2->getPosition(s1,w1);
cout<<"after move function"<<s1<<","<<w1<< p2->getDirection()<<endl;
}
}
This is plane class, the copy constructor and the move method.
Code:
enum DIRECTION
{
NORTH = 0,
EAST = 1,
SOUTH = 2,
WEST = 3
};
string _name;
int _x;
int _y;
DIRECTION _direction;
bool _hasLanded;
DIRECTION previous;
plane::plane(const std::string& name, int x, int y, DIRECTION direction)
{
_name=name;
_x=x;
_y=y;
_direction=direction;
previous = direction;
}
/**
Copy constructor
@param other Airplane to make copy of
*/
plane::plane(const plane& other)
{
_name=other.getName();
other.getPosition(_x,_y);
_direction=other.getDirection();
_hasLanded = other._hasLanded;
previous = _direction;
}
bool plane::move()
{
cout<<"plane move"<<_x<<","<<_y<<" "<<getDirection()<<endl;
switch(_direction)
{
case 0 :
_y++;
break;
case 1 :
_x++;
break;
case 2 :
_y= _y -1;
break;
case 3 :
_x=_x - 1;
break;
}
if(_hasLanded)
{
return false;
}
return true;
}
Re: Conditional jump depends on unitialized value Valgrind
A couple of things:
1) you have
Code:
if((*planeIteratorMoj)->move())
{
delete *planeIteratorMoj;
}
plane * p2 = (*planeIteratorMoj);
If the delete takes place, p2 is pointing to deleted memory. Also, what is the
purpose of p2 here ?
2) You do not need the copy constructor
3) in addPlane, what is the purpose of the "planes.empty()" check ?
Re: Conditional jump depends on unitialized value Valgrind
Quote:
Originally Posted by
fleg14
Hi, I am really lost here, I have airport class which should navigate planes, in its list to runways, with method move, theres a method prepare which changes the direction of flight to all planes, always before move is called, move just increments decrement x and y of plane in its list. But after calling two times in row airport->move(), I get screwed and I really dont know wheres the problem. Have I badly initiazed something? Iterator gets invalidated.
1) None of this code needs dynamic allocation. A
is all that is needed. Once you do that, then all of those calls to "new" and "delete" goes away. You are not deriving plane from a base class, so there is no reason to be dynamically creating and using plane*.
2) As Philip pointed out, you do not need a copy constructor, since all the members of plane are safely copyable by themselves. As a matter of fact, your copy constructor is faulty:
Code:
/**
Copy constructor
@param other Airplane to make copy of
*/
plane::plane(const plane& other)
{
_name=other.getName();
other.getPosition(_x,_y);
_direction=other.getDirection();
_hasLanded = other._hasLanded;
previous = _direction;
}
Look at the line in red. Why aren't you copying "previous" from other.previous? This is a bogus copy constructor, as there is no guarantee that this is an exact copy of other. This is why you don't write copy constructors if you don't have to -- let the compiler do the copying, as it will not make a mistake like the one you made above.
3) Where is your assignment operator to go along with the copy constructor? Better yet, don't write an assignment operator, since the compiler generated one will work OK without you writing it, the same way that the compiler generated copy constructor would have worked perfectly without you writing one.
4) Learn to use algorithm functions such as std::find_if() to search for particular planes, and std::for_each() to apply an action to every plane.
5) Don't start your member names with underscores. Names with underscores are reserved for the compiler's usage. Even if technically there is no problem with your code now, it is always a good thing to take precaution and not start names with underscores.
6) Learn to use initializer lists when creating your classes. Instead of this:
Code:
plane::plane(const std::string& name, int x, int y, DIRECTION direction)
{
_name=name;
_x=x;
_y=y;
_direction=direction;
previous = direction;
}
Use this:
Code:
plane::plane(const std::string& name, int x, int y, DIRECTION direction) :
name_(name), x_(x), y_(y), direction_(direction), previous_(direction)
{ }
This is assuming you change the member names to have underscores after the name instead of starting a name.
7) Where is your "airport" class?
Given all of the above points, here is a sample of an addPlane() that uses these concepts:
Code:
#include <list>
#include <algorithm>
class plane
{
// all of planes members here
};
typedef std::list<plane> PlaneList;
class airport
{
PlaneList planes;
public:
bool addPlane(const plane& p);
};
// a function object that gets called to compare two plane names and returns true
// if the names are equal, false otherwise
struct FindPlaneByName
{
FindPlaneByName(const plane& pl) : thePlane(pl) {}
bool operator()(const plane& p)
{ return p.getName() == thePlane.getName(); }
};
bool airport::addPlane(const plane& p)
{
// use find_if to search for a plane by name
if ( std::find_if(planes.begin(), planes.end(), FindPlaneByName(p)) == planes.end() )
{
// plane not found, so add it to list
planes.push_back(p);
return true;
}
return false;
}
Note how much shorter and to the point this looks compared to your "addPlane" function. Less lines, and each line of code is worded to actually mean what it does. The "find_if" finds a plane given a certain criteria. That criteria is the name, so a function object that gets called by find_if() is written that compares by name. Maybe a little advanced, but might as well see now how it can be done without waiting around and maybe never seeing it for yourself.
As to your plane::move() function: Where are the parameters to move the plane a certain distance? Why is it not this?
Code:
void plane::move(int xDirection, int yDirection)
{
//
}
Maybe you should explain a little better how this "plane::move" with no arguments or criteria as to how much to move the plane is supposed to work. In any event, the airport::move() would just move each plane in the list.
Regards,
Paul McKenzie
Re: Conditional jump depends on unitialized value Valgrind
Quote:
Originally Posted by
Philip Nicoletti
A couple of things:
1) you have
Code:
if((*planeIteratorMoj)->move())
{
delete *planeIteratorMoj;
}
plane * p2 = (*planeIteratorMoj);
If the delete takes place, p2 is pointing to deleted memory. Also, what is the
purpose of p2 here ?
2) You do not need the copy constructor
3) in addPlane, what is the purpose of the "planes.empty()" check ?
1) i put it there just for debbuging reasons, to watch the variables, I should learn how to debug in my IDE
2) Ok i will try it without it
3)in tasks it says when I add a plane, and lis is empty, the iterator must be set on the first plane, but when I add second plane i cant move with the iterator. (hope it makes sense)
To Paul, I have list<plane*> because you see how function addPlane looks, and this is how looks getCurrentPlane(I have an interaface so it has to look like this), if there is a way how to do it another way, I will be happy to get rid of dynamic allocation.
Code:
/**
Return current airplane (to which internal iterator points to)
@return airplane or NULL if planeCount == 0
*/
const plane* airport::getCurrentPlane() const //thats my const iterator u can find him in my first post
{
if(planes.size()== 0)
{
return NULL;
}
return *planeIterator;
}
2. I have a plane(const plane& other); in my interface, so I have to implement it
3. Goind to repear it.
4. Ok thanks. I am new to c++ and the c++ looks to me very confusing a user non-friendly, I cant look for a staff effectively there.
5. reworked.
Thanks for help, Can you look at the getCurrentPlane function if it is possible to rework it in some way that i can use list<plane> because it seems to me, impossible with the const plane* getCurrentPlane() const.
Thank you.
edit: plane::move, everytime u call move the plane will move for 1 step in its actual direction s if we have direction north plane does y++, if direction is WEST then x--;
