# Memory leak?[fixed][program at end]

This topic is 4402 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Hey everyone, I have this RPG Map test program, and I'm having a little trouble with it. It compiles fine, but in runtime, windows puts up an error about it, and it closes. "RPGMAPTEST.exe has encountered a problem and needs to close. We are sorry for the inconvenience." That is the error, here is the code.
//RPG Map Test
//V 0.1
#include <iostream>
#include <string>
using namespace std;
bool playing;
struct room {
int n, e, s, w;
string description;
};
struct map {
room MyMap[3][3];
room* curroom;
int currxpos;
int currypos;
void initMap();
string* getdescrip();
void changeroom(char in);
};
void map::initMap() {
MyMap[1][1].description = "A room. Exits: S and E.";
MyMap[1][1].e = 1;
MyMap[1][1].s = 1;
MyMap[1][2].description = "A hallway. Exits: S, E, N";
MyMap[1][2].n = 1;
MyMap[1][2].e = 1;
MyMap[1][2].s = 1;
MyMap[1][3].description = "A room. Exits: N and E.";
MyMap[1][3].n = 1;
MyMap[1][3].e = 1;
MyMap[2][1].description = "A yard. Exits W, E, and S.";
MyMap[2][1].e = 1;
MyMap[2][1].w = 1;
MyMap[2][1].s = 1;
MyMap[2][2].description = "A well. Exits N, E, S, W.";
MyMap[2][2].n = 1;
MyMap[2][2].e = 1;
MyMap[2][2].s = 1;
MyMap[2][2].w = 1;
MyMap[2][3].description = "A shop. Exits N, E, W";
MyMap[2][3].n = 1;
MyMap[2][3].e = 1;
MyMap[2][2].w = 1;
MyMap[3][1].description = "A bar. Exits S, W";
MyMap[3][1].s = 1;
MyMap[3][1].w = 1;
MyMap[3][2].description = "A yard. Exits N, W, S";
MyMap[3][2].n = 1;
MyMap[3][2].s = 1;
MyMap[3][2].w = 1;
MyMap[3][3].description = "A random tree. Exits N, W";
MyMap[3][3].n = 1;
MyMap[3][3].w = 1;
currxpos = 1;
currypos = 1;
curroom = &MyMap[currxpos][currypos];
}
string* map::getdescrip() {
return &curroom->description;
}
void map::changeroom(char in) {
if(in == 'X' || in == 'x') {
playing = false;
}
if(in == 'N' || in == 'n') {
currypos += 1;
}
if(in == 'S' || in == 's') {
currypos -= 1;
}
if(in == 'E' || in == 'e') {
currxpos += 1;
}
if(in == 'W' || in == 'w') {
currxpos -= 1;
}
curroom = &MyMap[currxpos][currypos];
}
void gameLoop();
map gamemap;
char input;
int main() {
playing = true;
while(playing == true) {
gameLoop();
}
return 0;
}
void gameLoop() {
cout<<"RPG MAP TEST."<<endl;
cout<<*gamemap.getdescrip()<<endl; //THIS LINE
cin>>input;
gamemap.changeroom(input);
}


Now however if I take the dereference operator off of the 'gamemap.getdescrip()' on the line labeled 'THIS LINE' towards the end. It works fine, but outputs the description's address in memory. So I try putting the dereference operator on, and it doesn't work in runtime. Help please? [Edited by - Niddles on December 29, 2005 1:08:32 AM]

##### Share on other sites
Couldn't you just return a plain 'string' instead of a pointer to the string?

##### Share on other sites
I've tried every possibility I can think of, and that was one of them, it still compiles, but gives that same run-time error, sadly.

##### Share on other sites
Have you tried using the "c_str()" function on the string (both with and without returing a pointer to the string)? I mean, when you 'cout' the function, have you tried:

cout << gamemap.getdescrip().c_str(); << endl;

##### Share on other sites
Using the c_str() function still causes a runtime error, with a normal string function, and with a string pointer function, it says c_str() is undeclared. What exactly does c_str() do? I've never dealt with it.

##### Share on other sites
This is not a memory leak problem, but an access violation.

A memory leak happens when you dynamically allocate some memory, but do not keep a pointer to that memory so that it can later be released. An access violation happens when you try to access memory that hasn't been properly allocated.

Array indices are zero-based: if you have room MyMap[3][3];, the coordinates can vary from 0 to 2, not from 1 to 3.

##### Share on other sites
That does make sense. I changed it so it was right, but I'm still getting a run time error, if I try to output the dereferenced return value of the getdescrip function. If I just output it it gives me the address of it. This seems like it would be easily solved by dereferencing the return value of getdescrip, but it just creates an error.

##### Share on other sites
Well, for what it's worth, it seems you are never calling initMap(), which would mean your data structure contains garbage, and its pointers point to somewhere in Access Violation Land.

##### Share on other sites
Hmmmm. Wow, what a miss. Thanks fruny, of all of the things for me to do wrong, that was it. Thanks alot =).

##### Share on other sites
Alright, here is my finished product, of my RPG Map Test. Tell me what you think of my programming design, and everything. Thanks for your help!
//RPG Map Test//V 0.1//Adam Martin#include <iostream>#include <string>using namespace std;bool playing;    struct room {     int n, e, s, w;    string description;};struct map {    room MyMap[3][3];    room* curroom;    int currxpos;    int currypos;    int num_errors;    void initMap();    string* getdescrip();    void changeroom(char in);    string error(string message);};string map::error(string message) {  num_errors++;  return message;}void map::initMap() {    MyMap[0][0].description = "A room. Exits: S and E.";    MyMap[0][0].e = 1;    MyMap[0][0].s = 1;    MyMap[0][1].description = "A hallway. Exits: S, E, N";    MyMap[0][1].n = 1;    MyMap[0][1].e = 1;    MyMap[0][1].s = 1;    MyMap[0][2].description = "A room. Exits: N and E.";    MyMap[0][2].n = 1;    MyMap[0][2].e = 1;    MyMap[1][0].description = "A yard. Exits W, E, and S.";    MyMap[1][0].e = 1;    MyMap[1][0].w = 1;    MyMap[1][1].s = 1;    MyMap[1][1].description = "A well. Exits N, E, S, W.";    MyMap[1][1].n = 1;    MyMap[1][1].e = 1;    MyMap[1][1].s = 1;    MyMap[1][1].w = 1;    MyMap[1][2].description = "A shop. Exits N, E, W";    MyMap[1][2].n = 1;    MyMap[1][2].e = 1;    MyMap[1][2].w = 1;    MyMap[2][0].description = "A bar. Exits S, W";    MyMap[2][0].s = 1;    MyMap[2][0].w = 1;    MyMap[2][1].description = "A yard. Exits N, W, S";    MyMap[2][1].n = 1;    MyMap[2][1].s = 1;    MyMap[2][1].w = 1;    MyMap[2][2].description = "A random tree. Exits N, W";    MyMap[2][2].n = 1;    MyMap[2][2].w = 1;    currxpos = 0;    currypos = 0;    curroom = &MyMap[currxpos][currypos];}string* map::getdescrip() {    return &curroom->description;}void map::changeroom(char in) {    if(in == 'X' || in == 'x') {        playing = false;    }    if(in == 'N' || in == 'n') {      if(curroom->n == 1) {        if(currypos - 1 < 0) {          cout<<error("You cant go there")<<endl;        }        else          currypos -= 1;      }      else        cout<<error("You cant go through a wall")<<endl;    }    if(in == 'S' || in == 's') {      if(curroom->s == 1) {        if(currypos + 1 > 2) {          cout<<error("You cant go there")<<endl;        }        else          currypos += 1;      }      else        cout<<error("You cant go through a wall")<<endl;    }    if(in == 'E' || in == 'e') {      if(curroom->e == 1) {        if(currxpos + 1 > 2) {          cout<<error("You cant go there")<<endl;        }        else          currxpos += 1;      }      else        cout<<error("You cant go through a wall")<<endl;    }    if(in == 'W' || in == 'w') {      if(curroom->w == 1) {        if(currxpos - 1 < 0) {          cout<<error("You cant go there")<<endl;        }        else          currxpos -= 1;      }      else        cout<<error("You cant go through a wall")<<endl;    }    curroom = &MyMap[currxpos][currypos];}void gameLoop();map gamemap; char input;int main() {    playing = true;    cout<<"RPG MAP TEST."<<endl;    gamemap.initMap();    while(playing == true) {    gameLoop();    }    return gamemap.num_errors;}void gameLoop() {    cout<<*gamemap.getdescrip()<<endl;    cin>>input;    gamemap.changeroom(input);}

##### Share on other sites
Well, first thing, you should really get the exit display string from the actual exit information stored in the data structure. That will avoid them getting out of sync. You also probably don't need to keep both the coordinates and the pointer, since one can be trivially derived from the other.

You should consider converting your code to load the map from a file in the near future. Hard-coding is no good. Also, you've been bitten by an uninitialized map, so it would be wise to learn about how to use constructors. [smile]

Storing the map in an array like that is also not very scalable. A std::deque of room pointers would be more convenient (just remember to clean up the pointers properly :p). You lose the 2D indexing, but that's generally no problem -- you just have to compute the index into the map from the coordinates.

An approach would be to give each room an identifying number and to rely on those numbers to connect the rooms to one another. That is, instead of using your 'exit' variables to indicate whether an exit is there or not, use them to indicate what room they lead to. Pointers would work great, but the identifier<->pointer conversion is probably going to be a bit too tricky for you yet. So you will have to reserve a value to indicate that there is no exit (like a null pointer indicates that there's no pointee).

As was mentioned earlier, returning pointers left and right is clunky. References (particularly, const references) are a good thing. Plus, some people take OOP and encapsulation to mean that you should have accessors for your members. I disagree. You already did something right by having a changeroom function rather than accessors that manipulate the individual coordinates. You should push it a bit further and add a member that prints the room description rather than return the description string: think in terms of operations rather than in terms of data access.

void map::print_description(ostream& out){   out << curroom->description << endl;   bool first_exit = true;      out << "Exits: ";   if (curroom->n) { out << "N"; first_exit = false; }   if (curroom->s) { out << first_exit ? "S" : ", S"; first_exit = false; }   if (curroom->e) { out << first_exit ? "E" : ", E"; first_exit = false; }   if (curroom->w) { out << first_exit ? "W" : ", W"; first_exit = false; }      out << first_exit ? "None." : "." << endl;}...gamemap.print_description(cout);

The bit of code above also shows that you might beneficiate from replacing the individual exit variables with an array or, later, a dictionary (std::map -- now that's going to be confusing if you don't change your class name [smile]).

##### Share on other sites
Quote:
 Original post by FrunyPlus, some people take OOP and encapsulation to mean that you should have accessors for your members. I disagree. You already did something right by having a changeroom function rather than accessors that manipulate the individual coordinates. You should push it a bit further and add a member that prints the room description rather than return the description string: think in terms of operations rather than in terms of data access.

Well said.

Quote:
 void map::print_description(ostream& out){ out << curroom->description << endl; bool first_exit = true; out << "Exits: "; if (curroom->n) { out << "N"; first_exit = false; } if (curroom->s) { out << first_exit ? "S" : ", S"; first_exit = false; } if (curroom->e) { out << first_exit ? "E" : ", E"; first_exit = false; } if (curroom->w) { out << first_exit ? "W" : ", W"; first_exit = false; } out << first_exit ? "None." : "." << endl;}...gamemap.print_description(cout);The bit of code above also shows that you might beneficiate from replacing the individual exit variables with an array or, later, a dictionary (std::map -- now that's going to be confusing if you don't change your class name [smile]).

Just for the heck of it:

enum {n, e, s, w} directions;const string directionNames = "NESW";const int directionCount = 4;struct room {   bool canMoveTo[directionCount];  string description;};void map::print_description(ostream& out) {  out << curroom->description << endl;  bool first_exit = true;     out << "Exits: ";  for (int i = 0; i < directionCount; ++i) {    if (curroom->canMoveTo[i]) {       if (!first_exit) { out << ", "; }      out << directionNames[i];      first_exit = false;    }  }  out << first_exit ? "None." : "." << endl;}ostream& operator<<(ostream& out, const map& m) {  m.print_description(out);  return out;}// ...cout << gamemap;

##### Share on other sites
Hmmm, I find your code very confusing. I don't understand what the boolean first_exit value is for. Also, how can I point to one of the spots on my map, without the current x and y positions?
Edit: Also, I was planning on putting my maps in txt files, I just wanted to get the map 'reader' working, and then I was going to put in a fstream to read in from the txt file. I have no experience in working with file streams, so I don't know how I'm going to go about it, but I'm going to learn.

##### Share on other sites
How's this for a start on filestream?
#include <iostream>#include <fstream>#include <string>using namespace std;int main() {  string lines[4];  ifstream my_in("map.txt");  if(my_in.is_open()) {    for(int i=0; i<4; i++) {      if(!my_in.eof()) {        getline(my_in, lines[i]);      }    }  }  my_in.close();  for(int i=0; i<4; i++) {    cout<<lines[i]<<endl;  }  return 0;}

##### Share on other sites

#include <iostream>#include <fstream>#include <string>using namespace std;int main() {  string lines[4];  int count = 0; // initialize to 0  register int i;  ifstream my_in("map.txt");  if(!my_in) // check if file opened or not  {    cout << "Error opening file map.txt" << endl;    return 1; // quit program  }  while(my_in.is_open() && !my_in.eof()) {        if(count == 5) break; // if we reach 5 break        count++; // increment our counter        my_in.getline(lines[i], 256); // not sure on this one might be less characters  }  my_in.close();  for(i=0; i<4; i++) {      cout<<lines[i]<<endl;  }  return 0;}

Some problems sorted there, i havn't tested this code so not sure if it works but i think it does.

##### Share on other sites
Quote:
 Original post by NiddlesHmmm, I find your code very confusing. I don't understand what the boolean first_exit value is for.

It's for indicating whether the exit being outputted is the first one. We don't want to put a comma before the first one.

Quote:
 Also, how can I point to one of the spots on my map, without the current x and y positions?

If you don't have the x and y positions, how do you know which spot you want to point to?

##### Share on other sites
Quote:
 Original post by GameMasterXL*** Source Snippet Removed ***Some problems sorted there, i havn't tested this code so not sure if it works but i think it does.

Do not use the member function std::istream::getline() to read into a string. It is intended for reading into character buffers (which is usually a bad idea anyway), hence the length count parameter. Use the free function std::getline() instead (as in the OP's code).

Also, you're reading into lines[0] each time because you index with i where you want to index with count. 'i' is really only useful for the loop here, and therefore should just be declared inline at the for loop.

Also, 'register' is totally useless with any decently modern compiler.

Also, if you're going to write comments, write useful ones. The OP clearly understands enough C++ to read your code.

Also, checking each time through if the file is 'still open' is silly. And getline() returns a reference to the stream, for chaining and also so that you can immediately check if the stream is ok.

Also, you missed that the .close() call is not actually needed (it will happen due to the ifstream's destructor; .close() is provided for situations when you need the close to happen before end of scope).

Also, you don't really fix any problems there except for providing a diagnostic message and bailing out if the file is not found. (In the OP's code, nothing would be read into the strings, but the string objects are still default-initialized, so nothing really bad will happen.)

Also, you don't fix the problem of the arbitrary limit on how many lines can be read.

#include <iostream>#include <fstream>#include <string>#include <vector>using namespace std;int main() {  vector<string> lines;  ifstream my_in("map.txt");  if (!my_in) {    cerr << "Couldn't open the file!" << endl;    return 1;  }  string current;  while (my_in.getline(current)) { lines.push_back(current); }  for (vector<string>::iterator it = lines.begin(); it != lines.end(); ++it) {    cout << *it << endl;  }}