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.

Shouldn't you be looping to the size instead of the top element? And do stacks normally have indexable elements? — chris, May 07 '13 at 02:27
And `top` is never set for the new object in the copy constructor. — syam, May 07 '13 at 02:28
Wait, why is `top` not a function (I'd argue the same for `size` as well if there isn't one)? — chris, May 07 '13 at 02:30
Sorry, I should have specified. I'm building a stack out of an array. So anytime you see the word stack, it's the class "stack" and it contains a dynamically allocated array. — Atrus, May 07 '13 at 02:33
Whoops. I do, but it looks like that didn't make it to the question when I copied the code. The code works most of the time, it's just special situations where it fails, namely situations where the copy constructor is used. — Atrus, May 07 '13 at 03:42
Please show the initialization code for the `size` member in your copy constructor — Pavel Zhuravlev, May 07 '13 at 03:47
The copy constructor seems to be ok now, provided that the `push()` is implemented correctly. The problem must be somewhere else in your code. Please post the actual code which causes seg fault or try debugging it to get more clues. — Pavel Zhuravlev, May 07 '13 at 04:11
Yeah, I thought it looked ok. I'm now convinced that the problem is somewhere in the destructor. The seg fault doesn't occur until the very end of the program when the end of main() is reached and the destructor would be called. — Atrus, May 07 '13 at 04:19
I tried your code out and I can't find anything wrong with your copy constructor. Memory faults can be tricky, sometimes they show up in places other than where the actual fault is. Can you provide more of your code and a stack trace. — Richard Johnson, May 07 '13 at 05:55

score 1 · Answer 1 · answered May 07 '13 at 02:44

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).

answered May 07 '13 at 02:44

Ozraptor

564
4
11

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 – Atrus May 07 '13 at 02:54
What type is items? Is it being declared as an int*? – Ozraptor May 07 '13 at 03:27
The code works most of the time, it's just special situations where it fails, namely situations where the copy constructor is used. – Atrus May 07 '13 at 03:41

score 1 · Answer 2 · answered May 07 '13 at 05:09

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).

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`? — Excelcius, May 07 '13 at 05:28
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. — molbdnilo, May 07 '13 at 10:11

Destructor causes Segmentation Fault

2 Answers2