Segmentation fault

Hello everybody, I know it's not easy to debug but maybe you could see some crazy errors that would be really easy to see, so I post it anyway...
Please help, I got to post this tonight and I can't manage to solve this bug
You can ask any questions...

#include <iostream>
#include "MyHashSet.h"

bool Included (const MyHashSet *setCover, const MyHashSet &W);
int MySetCover(const MyHashSet &W, MyHashSet setsArr[], int arrSize);
bool AllVisited(MyHashSet setsArr[], int arrSize);

// Creates a set cover and return the minimum number of subsets including the
// whole W.
// W is the set representing the world of elements.
// Sets arr are a lot of hash sets.
// Arrsize is the size of the setsArr.
int MySetCover(const MyHashSet &W, MyHashSet setsArr[], int arrSize)
{
int minimalSoFar=0;
int currentSize=0;
// Returns -1 if there is not a set cover.
int numberOfSets=0;

// We initialize it 100 so that it will work for the first iteration.
int minimumSize=100;
MyHashSet* setCover = new MyHashSet;
while (!Included(setCover, W))
{
// If we already visited all of setArr, there is no set cover.
if (AllVisited(setsArr, arrSize))
return -1;

// 1/ Looking for the Ui that gets most elements inside all that were
// not taken in W until A includes W.
// ie : the Ui that makes the set W.reduce(A.union(Ui)) minimal.
for (int i=0; i<arrSize; i++)
{
if (setsArr[i].IsVisited())
continue;
MyHashSet* tempUnion = setCover->Union(setsArr[i]);
MyHashSet* toAdd = W.Reduce(*tempUnion);
currentSize = toAdd->Size();
if (currentSize < minimumSize)
{
minimalSoFar = i;
minimumSize = currentSize;
}
delete tempUnion;
delete toAdd;
}

// 2/ Adding the best candidate so far and setting visited to true.
MyHashSet* setCoverUnion = setCover->Union(setsArr[minimalSoFar]);
delete setCover;
MyHashSet* setCover = new MyHashSet;

// 3/ Copy it to the "final set cover".
for (int i=0; i<HASH_SIZE; i++)
{
MyLinkedList* ll = setCoverUnion->GetLinkedList(i);
MyLinkedList::Iterator* it1 = new MyLinkedList::Iterator(ll);
while (it1->HasNext())
{
setCover->Add(it1->Next());
}
delete ll;
delete it1;
}

// 4/ Delete the temporary setCoverUnion.
delete setCoverUnion;
// Attention ICI.
setsArr[minimalSoFar].SetVisited();
numberOfSets++;
minimalSoFar=0;
minimumSize = 100;
}
delete setCover;
return numberOfSets;
}

// Check if all the subsets have been visited return true if yes.
bool AllVisited(MyHashSet setsArr[], int arrSize)
{
for (int i=0; i<arrSize; i++)
{
if (!setsArr[i].IsVisited())
return false;
}
return true;
}

// Returns true iff W is included in A.
bool Included (const MyHashSet *setCover, const MyHashSet &W)
{
// We need to check that everything that's inside W is also in setCover.
for (int i=0; i<HASH_SIZE; i++)
{
MyLinkedList* ll = W.GetLinkedList(i);
MyLinkedList::Iterator it(ll);
while (it.HasNext())
{
// Adding element of current list.
if (!setCover->InHashSet(it.Next()))
{
delete ll;
return false;
}
}
delete ll;
}
// W is included in A.
return true;
}

Thanks a lot
[4220 byte] By [cyberjoac] at [2007-11-20 11:04:39]
# 1 Re: Segmentation fault
What bug?
GCDEF at 2007-11-9 1:25:02 >
# 2 Re: Segmentation fault
First, use code tags when posting code. The code you have posted is practically unreadable.

Second, the code suffers from a very bad design. You are supposedly allocating memory in another function, and then deallocating in another function. Of course this will, at some point, break down. There is no way to know what breaks down, as this is a runtime problem that cannot be solved by just looking at code.

Third, you have totally unnecessary calls to "new" in your code. Are you a Java programmer by any chance?

MyHashSet* setCover = new MyHashSet;
//...
delete setCover;

This is unnecessary. You could have just done this:

MyHashSet setCover;

Then there is no need for "delete", and I think this is the crux of your entire problem -- overusage and misuage of the heap.

The first advice is to look at what you've coded, and correct the flaws:

for (int i=0; i<arrSize; i++)
{
if (setsArr[i].IsVisited())
continue;
MyHashSet* tempUnion = setCover->Union(setsArr[i]);
MyHashSet* toAdd = W.Reduce(*tempUnion);
currentSize = toAdd->Size();
if (currentSize < minimumSize)
{
minimalSoFar = i;
minimumSize = currentSize;
}
delete tempUnion;
delete toAdd;
}
How do we know that "tempUnion" or "toAdd" even points to dynamically allocated memory when delete is called? If it did, how do we know the proper deallocation routine to call (how do we know it isn't delete[])? How do we know that Union() is not a class factory, and you're not allowed to use "delete" on these values (albeit, the destructor would be protected or private if this were the case).

The function or class responsible for creating these objects should be the function or class that is responsible for destroying them. That is the way proper handling of dynamic memory should be done, and not the passing off of dynamically allocated memory to another function to handle the deletion.

Regards,

Paul McKenzie
Paul McKenzie at 2007-11-9 1:26:02 >