Thread: Exception error

Your destructor is wrong. You aren't passing the same pointer to delete[] that you got from new[]. This is your assertation failure.

You also need a copy constructor and an assignment operator; the automatically-generated ones will not work.

I imagine the program would work fine with those changes.

7stud has given you the simplest answer. It does have the drawback of (possibly) requiring more memory.

There is, due to how you implement, though, an algebraic relationship that always holds (assuming your push and pop are correctly done, more on that later). This relationship is:

originalStackPtr + counts = stackPtr.

This means you truly only need to keep two of these three as variables -- the third quantity is redundant and can always be generated from the other two. I.e.:

originalStackPtr = stackPtr - counts;
counts = stackPtr - originalStackPtr;
stackPtr = originalStackPtr + counts;

so by storing any two of these quantities, you have effectively stored all three.

Technically, the best one to leave out is count -- both count and stackPtr change their values with a push and pop, so omitting one of those two makes less work in the push() and pop() functions. But you could omit storing any of these three quantities and recover it whenever you needed, given the other two.

Edit:

There is also a problem (admittedly very subtle) with this code:

Code:

template<class T>
void Stack<T>::push(const T& a)
{
	if(counts<sizeOfstack)
	{
		*stackPtr++=a;
		counts++;
	}
	else throw StackExceptions::StackFull(sizeOfstack);
}

Can anyone correctly identify why the following code is preferable? That is, why/how could the first code fail but the second succeed?

Code:

template<class T>
void Stack<T>::push(const T& a)
{
	if(counts<sizeOfstack)
	{
		*stackPtr=a;
		stackPtr++;
		counts++;
	}
	else throw StackExceptions::StackFull(sizeOfstack);
}

Originally posted by noob2c
I think i got it

before deleting it I did this

stackPtr=0;
delete [] stackptr;

and now it works fine, is that the right thing to do by the way?

No; you're passing a null pointer to delete, which does nothing. You're leaking memory with every object.

The correct deletion would be "delete[] (stackPtr-counts);". If stackPtr has been incremented "counts" times, then the way to undo this is to subtract "counts".

Code:

----------------------------
dynamically allocated memory||            
----------------------------
           ^
           |
           |                         
       stackPtr                           NULL or 0

_______________
  other memory /
______________/

stackPtr = &of_something_else;

Code:

----------------------------
dynamically allocated memory||            
----------------------------
          

     stackPtr                            NULL or 0
         |
         |
         \/
________________
  other memory  /
_______________/

stackPtr = 0;

Code:

----------------------------
dynamically allocated memory||          
----------------------------    
                                          

     stackPtr   --------------------->  NULL or 0

________________
  other memory  /
_______________/

delete [] stackPtr;

That line doesn't delete the dynamically allocated memory. Remember when you delete a pointer, you are deleting the memory it points to *NOT* the pointer itself. So, it doesn't matter what pointer you delete as long as the pointer is pointing to the dynamically allocated memory. The goal is to delete the dynamically allocated memory not a certain pointer.

Cat, what is the subtle problem you posted? I don't get it.

Code:

*stackPtr++=a;

Wouldn't that increment *stackPtr before it assigns it to a?

Here is an explanation of the "subtle error". Consider again the original code:

Code:

template<class T>
void Stack<T>::push(const T& a)
{
	if(counts<sizeOfstack)
	{
		*stackPtr++=a;
		counts++;
	}
	else throw StackExceptions::StackFull(sizeOfstack);
}

Well, this does three things, and in this order:

* Increments stackPtr.
* Assigns a to the location pointed at by the old version of stackptr.
* Increments counts.

The other code does things in this order:

* Assigns a to the location pointed at by the old version of stackptr.
* Increments stackPtr.
* Increments counts.

Now, could any of these fail? Incrementing a pointer cannot fail, nor can incrementing an int. But, alas! -- the assignment operator. It can't fail *for primitive types*, but we're templated on this type, so it could be a class. And, if it is, it could throw.

In the first case, if assignment throws, stackPtr WILL still get incremented, the object we tried to assign to (which is now "on the stack") is invalid, and the count is wrong. The destructor will delete the wrong pointer and all the program ends in fiery doom.

In the second case, if the assignment throws, stackPtr will NOT get incremented, nor will count. We have a failed assignment, but the new element wasn't yet officially on the stack anyway (stackPtr wasn't incremented). We trust that whoever wrote the contained class made sure that failed assingments don't leave the object in an impossible or unusable state; that's nothing we can do anything about, the burden is on the author of the contained class. Ideally a failed assignment leaves the object in its pre-assignment state.

So it essentially did nothing: No increments, and the failed assignment didn't screw anything up (as long as the contained class is well-written).

Essentially, we want this function to do three things: assign and two increments. If we code this correctly, we should have an "all or nothing" success. If it can't accomplish all three, it should accomplish none of them. The first code does not; it could accomplish one of them but not the other two. The second code is "commit or rollback" because the push either succeeds completely or fails completely.

Wow, I never would have noticed.

Speaking of assignment, wouldn't it be better to use the approach the STL uses and initialize the new element using placement new instead of the assignment operator?
Would require a non-initializing memory allocation of course - but that might be a good thing anyway.