1

Here is the code of a simple program which is supposed to read a text file which contains one word per line, dynamically allocate memory needed to store all the words, print them on the screen and deallocate the memory used.

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

class Dict {
 public:
 int size;
 char ** words;
 Dict (int, int*);
 ~Dict ();
};

Dict::Dict(int s,int* sizes) {
 int i;
 size=s;
 words = new char* [s];
 for (i=0;i<s;i++)
  words[i] = new char [sizes[i]];
}

Dict::~Dict() {
 int i;
 for (i=0;i<size;i++) {
  delete [] words[i];
  printf("i=%d\n",i); // for debugging
  }
 delete [] words;
}

Dict loadDict (char* filename) {
 FILE* file;
 int n=0,i=0;
 int * sizes;
 char buff [64];

 file=fopen(filename,"r");
 while (!feof(file)) {
  n++;
  fscanf(file,"%*[^\n] \n");
 }
 sizes=new int [n];

 rewind(file);
 while (!feof(file)) {
  if (fscanf(file,"%s\n",buff)>0) {
   sizes[i]=strlen(buff);
   i++;
  }
 }

 rewind(file);
 Dict r(n,sizes);
 i=0;
 while (!feof(file)) {
  fscanf(file,"%s\n",r.words[i]);
  i++;
 }

 delete [] sizes;
 return r;
}

int main() {
 int i;
 Dict d=loadDict("dict.txt");
 for (i=0;i<d.size;i++)
 printf("%s|\n",d.words[i]);
 printf("%d DONE.\n",d.size);
 return 0;
}

The deallocating is done in the destructor of the Dict class. However, used on a sample text file with just a few words, the words are correctly printed, but the call to ~Dict crashes the application after the execution of 3 lines of the form delete [] words[i];. If I use Code::Block's debugger and set a breakpoint on that line and tell it to continue on each breakpoint, the program terminates normally.

Since this is a really simple program, I hope there is some kind of easy answer or fix!

user35359
  • 253
  • 1
  • 6
  • 1
    This is barely C++. If you're using C++, you might as well take advantage of some safety measures that are included in the standard library for you. – chris Nov 26 '12 at 22:32
  • 1
    Try error checking your code. – Aesthete Nov 26 '12 at 22:33
  • Seeing as how the answers mention the RoT, I present this: http://dl.dropbox.com/u/6101039/Modern%20C++.pdf – chris Nov 26 '12 at 22:37

4 Answers4

7

The Dict class has dynamically allocated members but uses the default copy constructor and assignment operator which results in two instances of Dict pointing to the same words array when a copy of Dict is made, which happens when the loadDict() function is used. When one of the Dict instances is destructed it leaves the other Dict instance with a dangling pointer and will result in a double deletion of the words array and its elements.

See What is The Rule of Three?

If this it not a learning exercise use a std::vector<std::string> instead along with C++ streams. For example:

std::vector<std::string> words;
...

std::ifstream in(filename);
std::string line;
while (in >> line) words.push_back(line);
Community
  • 1
  • 1
hmjd
  • 120,187
  • 20
  • 207
  • 252
3

You are not following the rule of three:

http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

Dict r is destructed at the end of loadDict while a shallow copy of itself was returned to main. At the end of main the pointers are deleted again.

As often in C++, you don't need pointers here. Store a std::vector<std::string> in Dict and there will be no tricks.

log0
  • 10,489
  • 4
  • 28
  • 62
1

What is definitely wrong is that your code doesn't have a copy constructor and you are returning an object by value. This can lead to double destruction and, hence, double deletion of the date. Whether this happens drpends on whether copy construction is elided or not.

Additionally, you allocate for each string just enough characters to hold the string but not the null terminator. That is, to store a string s with length strlen(s) you need to allocate strlen(s) + 1 characters.

I'd recommend using std::vector<std::string> instead: This avoids most of the problems.

Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
  • Thank you. The evil zero character was apparently the cause for the crash. – user35359 Nov 27 '12 at 04:29
  • The way you are returning your `Dict` object it is likely that the copy constructor is elided and, thus, I looked for another problem. Note that you also need to fix you copy constructor (and copy assignment)! There is no guarantee they are elided. – Dietmar Kühl Nov 27 '12 at 08:15
0

There are some issues in your codes:

  1. You are not checking if the fopen return a valid pointer back. (well I got this because I try to run the code without the txt file)

  2. You are returning a Dict obect, which will call the copy constructer and you didn't define one.

  3. You used a assignment operater for Dict but you didn't define one.

You can try to change loadDict() to just return an array of size, and then pass that array into your Dict constructor. In this way you don't have to write the copy constructor and the assignment operater.

laishiekai
  • 841
  • 1
  • 13
  • 26