Destructor causes Segmentation Fault

Question

I know there's a lot of similar questions out there, but I haven't found anything that helps yet. I've been at this for several hours now and it's driving me crazy. I get a segmentation fault when a destructor is called for a variable that was created by the copy constructor.

//Copy Constructor
Stack::Stack(const Stack &aStack)
{
   size = 0; //this is incremented as new items are pushed onto the stack.
   cap= aStack.cap;
   items = new int[aStack.cap]();
   for (int i = 0; i < aStack.size; i++)
      this->push(aStack.items[i]);  //Adds an item if not full and increments size
      // I have also tried: items[i] = aStack.items[i]
}

//Destructor
Stack::~Stack()
{
   cap= 0;
   size= 0;
   delete [] items;
   items = NULL;
}

I get the feeling I'm doing the copy constructor wrong, but I can't figure out what it is.

Solution 1

The line:

for (int i = 0; i < aStack.top; i++)

should be:

for (int i = 0; i < aStack.size; i++)

It's seg faulting because you are likely trying to access an out of range index or something similiar (undefined behaviour territory).

Solution 2

Well, I found it. I'll put it here for anyone as silly as I am (lacking basic c++ knowledge) so they don't spend hours looking for the same thing I was.

My problem was the destructor seemed to be working "selectively." It worked in some tests, but failed in others. After comparing the tests for hours, I finally found it.

The tests that were failing finished by testing the pop function, continuing until the stack was empty. In my destructor, there is a line which says delete [] items; This would have been fine except my pop function had a line which read items[size-1] = NULL; So in a way, every time pop removed an item, it was being NULLED/deleted before the destructor was called. Due to my very basic knowledge of c++, I was not aware that the delete command could not handle empty arrays. So, I simply got rid of the line that deleted the item ahead of time (essentially, to the end user, the item no longer exists because of encapsulation. The top index still changes so it's no longer accessible)

Anyway, final lesson: delete [] items; does not handle empty arrays (which makes sense I guess. The delete command is expecting an array much longer than my final array was turning out to be).

Comments

[Atrus]

If only it was that simple, I could have caught that :P The names are a bit inaccurate (can't do anything about it, my teacher picked them). "Top" is the index above the top element of the stack, or in other words, the size, and "size" is the capacity (think "maxSize"). So, top already iterates though all the elements that have been assigned. Thanks for the feedback though :D

[Ozraptor]

What type is items? Is it being declared as an int*?

[Atrus]

The code works most of the time, it's just special situations where it fails, namely situations where the copy constructor is used.

[Excelcius]

Can't tell for sure since I don't have the complete code but this doesn't sound like the right solution... NULL is a macro effectively defined as 0. So all this line did was set one of the integers in the items array to 0. It shouldn't have any effect on the delete in the destructor. Did you call delete items[size - 1] as well in pop?

[molbdnilo]

The assignment as such is not the reason - assigning NULL doesn't cause anything to be deleted. In particular, assigning NULL to an int will only set it to zero, and an array full of zeroes isn't "empty" in any way. More likely is that size-1 is an index outside the array - make sure that size is greater than zero when you do that.