Function call is crashing program

So, I have two classes defined, gramNode and molecule.

I have linked lists created, the nodes of which are of the class gramNode. The class molecule has some data, and it includes a pointer to a gramNode. So each object of the class molecule has a linked list of gramNodes attached to it.

Here are my class definitions.

Code:

class gramNode
{
      public:
                  gramNode(){count = 0, gram = " ", next = NULL;}
		  gramNode(string subString);
                  void setGram(string subString);
		  int getCount()const{return count;};
		  string getGram()const{return gram;};
		  int writeList(ofstream *osptr, int numGram);

      private:
                  string gram;
                  int nValue, count;
                  gramNode* next;
};

class molecule
{
      public:
                  molecule(){SMILES = " ", entropy = 0, head;}
		  molecule(string subString){SMILES = subString, entropy = 0, head;};
		  void setSMILES(string subString){SMILES = subString;};
		  void setGram(string gram){head->setGram(gram);};
		  void setPtr(){head = new gramNode;};
		  string getSMILES()const{return SMILES;};
		  void writeGrams(ofstream *osptr);

      private:
                  string SMILES;
                  double entropy;
                  gramNode* head;
};

each node has a string and an integer stored in it. I want to run down the linked list and output the string and number for each list. I had to do this earlier in the program, so I write this function. It's called from within a gramNode, and it does exactly what I described. I use this function elsewhere in my program and it works perfectly.

Code:

int gramNode::writeList(ofstream *osptr, int numGram)
{
	gramNode* seek = this;
	while (seek!= NULL)
	{
		*osptr << numGram << '\t' << seek->gram << '\t' << seek->count << endl;
		numGram++;
		seek = seek->next;
	}
	return numGram;
}

My problem is, when I want to output the list for a specific molecule. I thought the problem would be easy enough, just write a function that will go to the 1st node attached to the molecule and evoke the writeList function. So, I wrote this.

Code:

void molecule::writeGrams(ofstream *osptr)
{
	head->writeList(osptr, 1);
}

the 1 is something left over from when I used the function earlier in the program. Sometimes it's important to keep track of that number, other times it isn't. So in this case, I just set it to 1 and let it run.

Code compiles and builds fine, and then the program crashes when I run it.


Any help would be greatly appreciated, thanks guys.

Probably a problem in some other part of the code.

Your design is somewhat problematic. For example:

Code:

molecule(){SMILES = " ", entropy = 0, head;}
molecule(string subString){SMILES = subString, entropy = 0, head;};

Do you think that does something good for head? (It just leaves it uninitialized as it was before).

Code:

void setPtr(){head = new gramNode;};

What happens to the old linked list pointed to by head if any?

Code:

void setGram(string gram){head->setGram(gram);};

What happens if head happens to be uninitialized?

Also, where do you release all the memory you allocate?

If you can, it might be a lot simpler just to use an instance of the std::list<gramNode> for a linked list in molecule and remove all the manual linked list code.

P.S You don't need a semicolon after the closing brace of a function definition.

Originally Posted by anon

Probably a problem in some other part of the code.

Your design is somewhat problematic. For example:

Code:

molecule(){SMILES = " ", entropy = 0, head;}
molecule(string subString){SMILES = subString, entropy = 0, head;};

Do you think that does something good for head? (It just leaves it uninitialized as it was before).

I think this was what was causing the program to crash, those should have been initialized to NULL. My mistake.


I fixed it, and while the program doesn't crash anymore, it's not outputting to the file correstly. Still, I should be able to figure this out now...

Man, I don't know how I missed that. Thanks for the catch.

Originally Posted by anon

Code:

void setPtr(){head = new gramNode;};

What happens to the old linked list pointed to by head if any?

This function is only called when I have a need to attach a linked list to a molecule. Before this function is called, head should be NULL. Now that I changed the initializers, this should be the case.

Do you think I should include a check to make sure it is equal to NULL first? Yes, that's probably a good idea...

Originally Posted by anon

Code:

void setGram(string gram){head->setGram(gram);};

What happens if head happens to be uninitialized?

Fixed.

Originally Posted by anon

Also, where do you release all the memory you allocate?

Haven't gotten there yet. I should probably do that. This is my first time using classes, dynamic arrays and linked lists, so I forgot about those, especially since the book I'm using talks about all that, and then 50 pages later has a small section on destructors. Guess I overlooked it. thanks for the heads up.

Originally Posted by anon

If you can, it might be a lot simpler just to use an instance of the std::list<gramNode> for a linked list in molecule and remove all the manual linked list code.

P.S You don't need a semicolon after the closing brace of a function definition.

How much more simple? Would it be faster?

I ask because like I said, I am still in my early learning stages, so I designed this to both be functional and as an exercise in setting up my own classes and data structures.



The code's not crashing anymore. It's not working, but it's not crashing. I'll try to figure out what else is wrong. Thanks for your help, much appreciated.

I think I'm not building the list correctly, which is frustrating since I'm using the same functions I use elsewhere, and they work there.

How much more simple? Would it be faster?

I ask because like I said, I am still in my early learning stages, so I designed this to both be functional and as an exercise in setting up my own classes and data structures.

Simpler as in you get a linked list implementation that is pretty much guaranteed not to have any bugs for free.

E.g adding a gram to a molecul:

Code:

void molecul::add_gram(const std::string& name)
{
     //std::list has a size member, but it has linear complexity.
    //so unless you don't do any advanced splicing or merging you may keep track of it yourself 
    ++gram_count;

    gram_list.push_back(gram(gram_count, name));
}

E.g outputting the list:

Code:

void molecul::write_list() const
{
    for (std::list<gram>::const_iterator it = gram_list.begin(); it != gram_list.end(); ++it) {
        it->write(); //assuming gram has a write method to output its own data
    }

   //or, if you are feeling like learning to use the algorithms header effectively, it might be better
   //to write the same thing as:

    std::for_each(gram_list.begin(), gram_list.end(), std::mem_fun_ref(&gram::write));
}

std::list is doubly linked, so there may be a small extra memory overhead (but I think many compilers also provide a non-standard slist - singly linked list). But I really wouldn't worry about it too early. Depending on what you use the list for, it might be replaced with a more suitable container.

It is useful to know how to implement a linked list yourself (knowledge is good), but it is a waste of time to re-implement a specific linked list every time you need one. Studying std::list might also give you an idea how to write a reusable generic linked list (but once you know how to do that you should use std::list - or other standard containers - unless you have a very good reason not to).