Memory leak - need help finding

Alright guys, I have a class called Bible. I have a memory leak when I create a new Bible object (being a pointer), then delete the object, the recreate it using a different file bible source file for the constructor parameter.

It's about 4.5 - 5MB, and I don't know what I'm doing wrong. I also have Book, Chapter, and Verse classes which go along with the Bible class. It's all a tree structure.

I'm thinking maybe it's cause a lot of my class members are not pointers, just normal stack variables, so they're not being deleted properly when the object is destroyed...?

Here's the source file. Any insight would be wonderful.

*EDIT*
nevermind the source file, I'll just post the code here.

Code:

---

/******************************************************************************
 *
 * This file is part of BibleReader.
 *
 * BibleReader is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 *
 */

#ifndef BIBLESTRUCTURES_H
#define BIBLESTRUCTURES_H

#include <iostream>
#include <sstream>
#include <fstream>
#include <vector>
#include <queue>

using namespace std;

// class prototypes
class SearchResult;
class Verse;
class Chapter;
class Book;
class Bible;

// constants
const int LENGTH_BYTES = 2;
const int LOCATION_BYTES = 4;
const int COUNT_BYTES = 2;

class SearchResult
{
 private:
    int book;
    int chapter;
    int verse;

 public:
    SearchResult(int b, int c, int v)
    {
        book = b;
        chapter = c;
        verse = v;
    }

    ~SearchResult()
    {
    }

    const int GetBook() const
    {
        return book;
    }

    const int GetChapter() const
    {
        return chapter;
    }

    const int GetVerse() const
    {
        return verse;
    }
};


class Verse
{
 private:
    int pos;
    string text;

 public:
    Verse(const int p)
    {
        pos = p;
    }

    Verse(const int p, string t)
    {
        pos = p;
        text = t;
    }

    ~Verse()
    {
    }

    const int GetVersePos() const
    {
        return pos;
    }

    string GetText()
    {
        return text;
    }
};


class Chapter
{
 private:
    int pos;
    int nVerses;
    vector<Verse*> verse_list;

 public:
    Chapter(const string *buffer, long p);

    ~Chapter()
    {
    }

    const int GetVerseCount() const
    {
        return nVerses;
    }

    const int GetChapterPos() const
    {
        return pos;
    }

    Verse *GetVerse(int v) const
    {
        if (v > nVerses || v < 0)
            return NULL;

        return verse_list[v];
    }
};


class Book
{
 private:
    string book_name;
    int pos;
    int nChapters;
    vector<Chapter*> chapter_list;

 public:
    Book(const string *buffer, long p);

    ~Book()
    {
    }

    const string GetBookName() const
    {
        return book_name;
    }

    const int GetChapterCount() const
    {
        return nChapters;
    }

    const int GetBookPos() const
    {
        return pos;
    }

    Chapter *GetChapter(int c) const
    {
        if (c > nChapters || c < 0)
            return NULL;

        return chapter_list[c];
    }
};


class Bible
{
 private:
    string title;
    string copyright;
    vector<Book*> book_list;
    int nBooks;
    string *buffer;

 public:
    Bible(const char *fname);

    ~Bible()
    {
    }

    bool InputFileToMemory(const char *fname);
    queue<SearchResult*> *Find(string str);

    const string GetTitle() const
    {
        return title;
    }

    const string GetCopyright() const
    {
        return copyright;
    }

    Book *GetBook(int b) const
    {
        if (b > nBooks || b < 0)
            return NULL;

        return book_list[b];
    }

    const int GetBookCount() const
    {
        return nBooks;
    }

    const string GetVerseText(int b, int c, int v) const
    {
        if (b > nBooks || b < 0)
            return string("");

        if (c > GetBook(b)->GetChapterCount() || c < 0)
            return string("");

        if (v > GetBook(b)->GetChapter(c)->GetVerseCount() || v < 0)
            return string("");

        return GetBook(b)->GetChapter(c)->GetVerse(v)->GetText();
    }
};

Chapter::Chapter(const string *buffer, long p)
{
    istringstream tmp_buffer;
    long verse_pos = 0;
    int len = 0;
    string verse_text = "";
    int i = 0;

    // store chapter location incase needed for future reference
    pos = p;
    nVerses = 0;

    // number of verses in chapter
    tmp_buffer.str(buffer->substr(p, COUNT_BYTES));
    tmp_buffer.read((char*)&nVerses, COUNT_BYTES);
    p += COUNT_BYTES;

    // set up verses
    for (i=0; i<nVerses; i++)
    {
        // verse location
        tmp_buffer.str(buffer->substr(p, LOCATION_BYTES));
        tmp_buffer.read((char*)&verse_pos, LOCATION_BYTES);
        p += LOCATION_BYTES;

        // check that verse has not been left out of Bible. If it has, insert an empty string for the verse text
        if (verse_pos > 0)
        {
            // verse text length
            tmp_buffer.str(buffer->substr(verse_pos, LENGTH_BYTES));
            tmp_buffer.read((char*)&len, LENGTH_BYTES);

            // verse text
            verse_text = buffer->substr(verse_pos+LENGTH_BYTES, len);

            // insert new book into book list
            verse_list.push_back(new Verse(verse_pos, verse_text));
        }
        else
            verse_list.push_back(new Verse(verse_pos, string("")));
    }
}

Book::Book(const string *buffer, long p)
{
    istringstream tmp_buffer;
    long chapter_pos = 0;
    int len = 0;
    int i = 0;

    // store book location incase needed for future reference
    pos = p;
    nChapters = 0;
    book_name = "";

    // book name length
    tmp_buffer.str(buffer->substr(p, LENGTH_BYTES));
    tmp_buffer.read((char*)&len, LENGTH_BYTES);
    p += LENGTH_BYTES;

    // book name
    book_name = buffer->substr(p, len);
    p += len;

    // number of chapters in book
    tmp_buffer.str(buffer->substr(p, COUNT_BYTES));
    tmp_buffer.read((char*)&nChapters, COUNT_BYTES);
    p += COUNT_BYTES;

    // set up chapters
    for (i=0; i<nChapters; i++)
    {
        // chapter location
        tmp_buffer.str(buffer->substr(p, LOCATION_BYTES));
        tmp_buffer.read((char*)&chapter_pos, LOCATION_BYTES);
        p += LOCATION_BYTES;

        // insert new book into book list
        chapter_list.push_back(new Chapter(buffer, chapter_pos));
    }
}

Bible::Bible(const char *fname)
{
    istringstream tmp_buffer;
    long buffer_offset = 0;
    long book_pos = 0;
    int len = 0;
    int i = 0;

    // TODO: better error checking here
    if (!InputFileToMemory(fname))
        return;

    // title length
    tmp_buffer.str(buffer->substr(buffer_offset, LENGTH_BYTES));
    tmp_buffer.read((char*)&len, LENGTH_BYTES);
    buffer_offset += LENGTH_BYTES;

    // title
    title = buffer->substr(buffer_offset, len);
    buffer_offset += len;

    // copyright length
    tmp_buffer.str(buffer->substr(buffer_offset, LENGTH_BYTES));
    tmp_buffer.read((char*)&len, LENGTH_BYTES);
    buffer_offset += LENGTH_BYTES;

    // copyright
    copyright = buffer->substr(buffer_offset, len);
    buffer_offset += len;

    // number of books
    tmp_buffer.str(buffer->substr(buffer_offset, COUNT_BYTES));
    tmp_buffer.read((char*)&nBooks, COUNT_BYTES);
    buffer_offset += COUNT_BYTES;

    // set up books
    // get the book locations, then create a new Book object and let do the rest
    for (i=0; i<nBooks; i++)
    {
        // book location
        tmp_buffer.str(buffer->substr(buffer_offset, LOCATION_BYTES));
        tmp_buffer.read((char*)&book_pos, LOCATION_BYTES);
        buffer_offset += LOCATION_BYTES;

        // insert new book into book list
        book_list.push_back(new Book(buffer, book_pos));
    }

    delete buffer;
}

// cache bible file in memory for faster access
// return true on success, false on error
bool Bible::InputFileToMemory(const char *fname)
{
    fstream in_file;
    char *tmp_buffer;
    long fsize;

    in_file.open(fname, ios::in|ios::binary|ios::ate);

    if (in_file == NULL)
        return false;

    // get size of file
    fsize = in_file.tellg();
    in_file.seekg(0, ios::beg);

    tmp_buffer = new char[fsize];

    // read file into buffer
    in_file.read(tmp_buffer, fsize);
    in_file.close();

    buffer = new string(tmp_buffer, fsize);

    delete tmp_buffer;

    return true;
}

queue<SearchResult*> *Bible::Find(string str)
{
    queue<SearchResult*> *results;
    int i, j, k;

    results = new queue<SearchResult*>;

    for (i=0; i<GetBookCount(); i++)
    {
        for (j=0; j<GetBook(i)->GetChapterCount(); j++)
        {
            for (k=0; k<GetBook(i)->GetChapter(j)->GetVerseCount(); k++)
            {
                // check that verse has not been left out of Bible (empty string). If it has, go to the next verse
                  if (GetVerseText(i, j, k).length() == 0)
                    continue;

                if (GetVerseText(i, j, k).find(str) != -1)
                    results->push(new SearchResult(i, j, k));
            }
        }
    }

    return results;
}

#endif

---

Hmm...yup, a lot of new's that don't get deleted.

Code:

---

            verse_list.push_back(new Verse(verse_pos, verse_text));

---

This allocates your memory. In your class destructor, you need to go back through your verse_list and delete it. The vector class will not do this for you. Similarly with the chapter_list and book_list.

Code:

---

queue<SearchResult*> *Bible::Find(string str)
{
    queue<SearchResult*> *results;
    int i, j, k;

    results = new queue<SearchResult*>;
    //snip
   
    return results;
}

---

I understand what you're doing here. It's bad. That leaves it up to the outside world to delete the results. Instead, restructure your function declaration to use an output parameter:

Code:

---

void Bible::Find(string str, queue<SearchResult*>& results)

---

*EDIT* deleting the vector components when the objects were deleted definitely worked. Thanks.

So with an output parameter like that, being by reference, will the outside not have to delete it? Who deletes it?

Also, SHOULD I make every class member a pointer?

Quote:

---

Originally Posted by ChadJohnson

So with an output parameter like that, being by reference, will the outside not have to delete it? Who deletes it?

---

Well, that's a good point. I did miss that the queue was yet again storing pointers. The SearchResult class is relatively small. I suggest trying something like this:

Code:

---

void Bible::Find(string str, queue<SearchResult>& results)
{
    //snip
                    if (GetVerseText(i, j, k).find(str) != -1)
                        results->push(SearchResult(i, j, k));
}

---

Then there's no messy allocating or deallocating of memory necessary. To call from the outside, all you need is something like this:

Code:

---

int main()
{
  Bible theBible;
  string searchQuery;
  queue<SearchResult> output;
  //do stuff
  theBible.Find(searchQuery,output);
  //do other stuff
  return 0;
}

---

Quote:

---

Originally Posted by ChadJohnson

Also, SHOULD I make every class member a pointer?

---

I wouldn't. There's no real need.

The SearchResult class is used in conjunction with a GUI control - a list box. One object is stored for each item in the list, so there can be up to 30000 SearchResult objects in a list.

So do you think I should do what you said in your last post and still not worry abut making the objects in the queue pointers, just stack variables?

Look at it this way. No pointers = 30000 SearchResult objects in the queue. Pointers = 30000 SearchResult pointers in the queue + 30000 SearchResult objects floating around elsewhere.

I'd just like to point out here, they won't be stack variables no matter what you do. I'm pretty sure queue uses dynamic memory internally, so it will all go on the heap anyway.

The main concern in deciding between storing pointers and storing objects is usually:
a) Does the object have a suitable copy constructor/assignment operator? i.e. a copy of the original object will function in exactly the same way, with no shared resources. If not, then you must use pointers.
b) Is the object very large? If so, you may lose efficiency in lengthy copy operations.
b2) Is the code going to be run many times per second? If so, depending on how often objects are added/deleted, you may or may not notice slowdowns.

In your case, the object is composed of 3 ints; (a) the default copy constructor/assignment operator will suffice, since everything is by-value and contained within the structure. (b) 3 ints is pretty much insignificant unless you're running it several billion (trillion?) times per second for an extended period of time. Looks to me like it'll be run 30,000 times max. Thus (b2) is also insignificant.

I conclusion, I would recommend storing objects in the queue, since: (a) It makes life much easier, and (b) It makes life safer, and (c) Negative side-effects are negligible.

:)

Can you explain what you mean by 'shared resources'? Also, what do you mean by an assignment operator?

>>Also, what do you mean by an assignment operator?

Code:

---

x = y; //Assignment operator of x is called, to copy y's contents into itself.

---

>>Can you explain what you mean by 'shared resources'?
Perhaps 'shared resources' isn't a perfectly accurate description. I'm not sure how much you know about the subject already, but an example of what I mean is dynamic memory. If a direct copy is made of the pointer, then both objects will access the same memory and you'll get data corruption (assuming you're not expecting it). A common example for this is dynamic memory that is freed by a destructor; if you push such an object onto a container, a copy of it is made, and when the original goes out of scope, the dynamic memory is deleted and you have a dangling pointer left in the object in the container. In this case, if the class is simple enough (or well enough designed), it should be an easy task to create a copy constructor that creates its own dynamic memory and copies the value rather than the address. However, there are cases where a copy simply will not suffice, i.e. when the resource in question cannot be copied without losing information or it would be extremely inefficient/difficult to do such a copy - such as when the resource is a large tree structure. In these cases, you require a workaround such as reference counting, so that the copied object will reference the same resource but the resource itself will not be freed until all 'copies' of the object are destroyed. Depending on the situation, this may be overkill (or you might be too lazy to do it ;)), and as a result we often have structures that cannot be reliably copied etc. for direct use in STL containers; this problem is solved by storing pointers to dynamically allocated objects instead.