Passing around a pointer

[Joking]

Hello,

I'm having issues when trying to pass around a pointer. Here's the code, explanation and question afterwards...

...

class CommonList
{
private:
string *words;
int numWords;
int size;
...

.........................................


CommonList::CommonList(int size)
{
this->words = NULL;
this->numWords = 0;
this->size = 50;
}

CommonList::CommonList(int size)
{
this->words = new string[size];
this->numWords = 0;
this->size = size;
}

CommonList::~CommonList()
{
}

CommonList::CommonList(const CommonList &original)
{
this->words = new string[this->size];

for (int i = 0; i < original.numWords; i++)
{
this->words[i] = original.words[i];
}
this->numWords = original.numWords;
this->size = original.size;
}

CommonList& CommonList::operator=(const CommonList &original)
{
delete this->words;
this->words = new string[this->size];

for (int i = 0; i < original.numWords; i++)
{
this->words[i] = original.words[i];
}
this->numWords = original.numWords;
this->size = original.size;
return *this;
}




void CommonList::addWord(string word)
{
this->words = &word;
this->numWords++;
}



.........................................
class Writer: public CommonList
{
private:
string name;
CommonList *commonlist

...

.........................................


Writer::Writer()
{
this->name = "";
this->commonlist = new commonlist[50];
}

Writer::Writer(string name)
{
this->name = name;
this->commonlist = new commonlist[50];
}

Writer::Writer(const Writer &original)
{
this->name = original.name;
this->commonlist = original.commonlist;
}

Writer& Writer::operator=(const Writer &original)
{
delete this->commonlist;
this->commonlist = new CommonList[50];
return *this;
}

Writer::~Writer()
{
}

void Writer::acceptCommonList(CommonList *list)
{
this->commonlist = list;
}


CommonList* Writer::handOverCommonList()
{
CommonList *list = NULL;
list = this->commonlist;
return list;
}


.........................................

In main, an array of writers are created: Writer *writers = new Writer[numWriters];

The idea is for the first writer to write down a word on the list, and then pass the list on to writer number two, who also writes a word and then passes the list to writer number three, etc. Codewise, the idea is to pass around the pointer (the memory address to the object), rather than copying the object itself everytime it moves on to the next writer…

If the first writer (at place 0 in the writers array) passes the list to the next player, it would be done like this (although not using static values like [0]):

writers[1].acceptCommonList(writers[0].handOverCommonList())

Something goes wrong here. These functions don’t make any fuss per se, I can run through them indefinitely without any issues. But if I then try to access the words that are supposed to be found via the commonlist array, e g:

cout << commonlist[0].getWord(0) << endl;

…I get an error (c0000005).

So I’m thinking that either there is something wrong in the way a word is added, or with the copy constructor, or with the overloading of the assignment operator, or…

Can anyone see where this goes wrong and what one would have to do to make it work properly?

[Disch]

I don't want to be harsh, but there are tons of problems with this code. You have dropped pointers, shared ownership of memory, memory leaks, and a half dozen other major issues. I recommend scrapping this entirely and rewriting from scratch.

Your biggest problem is memory management related. Whenever you allocate memory, it needs to be cleaned up, and to keep track of what memory has been allocated, it's often best for each allocation to have a clear "owner". That owner is responsible for allocating and freeing the memory. When you start passing allocated blocks around and there's no clear ownership, you'll run into memory leaks and other issues that you're seeing.

You also seem to be misunderstanding the concept of inheritance. Why is Writer derived from CommonList? It doesn't seem to make sense in this context. Especially since Writer seems to have its own array of CommonLists. Inheritance forms an "is a" relationship. For example, deriving "Poodle" from "Dog" makes sense, because a Poodle "is a" Dog. This allows you to treat specific types of dogs generically (ie: if you have Poodle and BlackLab both deriving from Dog, you can write code that uses Dogs and it will work with all dogs the same -- not just specific types of dogs).

Here, it doesn't seem like Writer is a CommonList, so inheritance seems faulty.



Also, you can avoid lots of these headaches by avoiding dynamic allocation entirely. It's generally a good idea to try and avoid it unless you need it.

The idea is for the first writer to write down a word on the list, and then pass the list on to writer number two,

The first writer should know nothing about the second writer - it shouldn't even be aware of its existence. Whoever owns the writers and the list should be the one in charge of passing the list around (in this case, main would probably be the owner)



From what your description sounds like... you want all writers to share a common list of words. This is actually very easy to do.

1) You need 1 list (owned by something higher up than each Writer -- probably owned by main)
2) You need X Writers (probably also owned by main)
3) You need the owner of those objects to give the Writers a pointer to the list.
--) Writers have no need to do any memory management
--) Use a container class for the list so it also doesn't have to do any memory management.


A simple way to do this:

#include <list>  // for std::list

typedef list<string>  WordList;

class Writer()
{
private:
  WordList* theList;

public:
  void SetWordList( WordList* v )
  {
    theList = v;
  }

  void IWantToAddAWordToTheList()
  {
    theList->push_back( "my word" );
  }
};


int main()
{
  WordList theOnlyWordList;  // main owns the word list
  vector<Writer> theWriters( number_of_writers );  // the array of writers

  for(int i = 0; i < number_of_writers; ++i)
    theWriters[i].SetWordList( &theOnlyWordList );  // give the word list to all writers


  // ... you're done!
}

Notice:
- no need for new/delete
- minimal pointers getting passed around
- much simpler and easier to follow

[cire]

There are a multitude of problems with this code.

First among them: Writer seems to think it owns what it's CommonList* points to. In most cases the memory is leaked, but in operator= you actually destroy a CommonList and you have no way of knowing whether that CommonList was created by the Writer itself or if it was passed in previously via acceptCommonList. You also use the wrong version of delete in operator=. Nowhere in the code given does a Writer actually add a word to a CommonList.

void CommonList::addWord(string word)
{
this->words = &word;
this->numWords++;
}

In addWord, the parameter word is a local variable that goes out of scope when the function returns. So, storing it's address is wrong. The address ceases being valid as soon as the function ends. Also, you lose access to any other words that may have been in the list here. If words was previously pointing to memory occupied by other words, how do you access that memory now?

CommonList::operator= uses the wrong version of delete. After you incorrectly delete the memory, you allocate enough memory to contain this->size strings, however, you then change this->size to original.size. Do you see a problem with that?

~CommonList doesn't free any memory.

In the CommonList copy constructor you use this->size before it has any meaningful value, so it could be anything.

You have two versions of the constructor taking a size parameter. One of them doesn't allocate any memory for the list and sets the size inappropriately.

[Joking]

Disch, no problem, I was expecting a good smack-up for posting this code, so I had it coming already :)

Guys, thanks so much for you replies. I'll return to my coding chamber and get back when I get this working (which will probably take quite some time).

Best,

J

[Joking]

Guys, here's my feedback on how I solved it, still using dynamic allocation. I'd rather keep things simple too, but sometimes (other than this) there is a point to using dynamic memory allocation, for optimising memory usage when working with large sets and static allocation would eat loads of memory. So I needed to work on this in order to learn some of the how-to. Thanks for pointing out non-dynamic solutions though. And for the other suggestions of course.

Here is the code as it looks after fixing it up, works without leakage or other issues now.


CommonList::CommonList(int size)
{
this->numWords = 0;
this->size = 50;
this->words = new string[size];
}

CommonList::CommonList(int size)
{
this->numWords = 0;
this->size = size;
this->words = new string[size];
}

CommonList::~CommonList()
{
delete [] this->words;
}

CommonList::CommonList(const CommonList &original)
{
this->words = new string[this->size];

for (int i = 0; i < original.numWords; i++)
{
this->words[i] = original.words[i];
}
this->numWords = original.numWords;
this->size = original.size;
}

CommonList& CommonList::operator=(const CommonList &original)
{
delete [] this->words;
this->words = new string[this->size];

for (int i = 0; i < original.numWords; i++)
{
this->words[i] = original.words[i];
}
this->numWords = original.numWords;
this->size = original.size;
return *this;
}

void CommonList::addWord(string word)
{
this->words[numWords] = word;
this->numWords++;
}


.........................................
class Writer: public CommonList
{
private:
string name;
CommonList *commonlist

...

.........................................


Writer::Writer()
{
this->name = "";
this->commonlist = NULL;
}

Writer::Writer(string name)
{
this->name = name;
this->commonlist = NULL;
}

Writer::Writer(const Writer &original)
{
this->name = original.name;
this->commonlist = new commonlist();
for (int i = 0; i < 1; i++)
{
this->commonlist[i] = original.commonlist[i];
}
}

Writer& Writer::operator=(const Writer &original)
{
delete this->commonlist;
this->name = original.name;
this->commonlist = new commonlist();
for (int i = 0; i < 1; i++)
{
this->commonlist[i] = original.commonlist[i];
}
return *this;
}

Writer::~Writer()
{
delete this->commonlist;
}

void Writer::acceptCommonList(CommonList *list)
{
this->commonlist = list;
}


CommonList* Writer::handOverCommonList()
{
CommonList *list = NULL;
list = this->commonlist;
this->commonlist = NULL;
return list;
}