Click here to Skip to main content
15,867,568 members
Please Sign up or sign in to vote.
0.00/5 (No votes)
See more:
Hi,

my code below adds 5 instances of a class to a vector and then uses erase() to remove the vector's first item. erase() really removes the first item. But it deletes the 5th!
Tried it in Visual Studio and Xcode/GCC. What am I doing wrong?

Thanks,

Thomas

--- code: ---
#include "stdafx.h"
#include <vector>

bool bVectorErase = false;

class c
{
public:
	c()
	{
		pStr = 0;
	}
	~c()
	{
		if(pStr)
			delete pStr;
	}
	//copy constructor:
	c(const c & other)
	{
		pStr = 0;
		SetI(other.i);
	}
	//in will be passed to the struct's int and string
	void SetI(int in)
	{		
		i = in;
		if(pStr)
			delete pStr;
		pStr = new char [10];
		sprintf(pStr, "%d", in);
	};
	int i;
	char * pStr;
};

void TestVectorErase()
{
	long ix;
	std::vector<c> vctc;
	for(ix = 0; ix < 5; ix ++)
	{
		c cinst;
		cinst.SetI(ix);
		vctc.push_back(cinst);
	}
	bVectorErase = true;
	char * pShouldBeDeleted = vctc[0].pStr;
	char * pShouldNotBeDeleted = vctc[4].pStr;
	vctc.erase(vctc.begin());
}
Posted
Updated 7-Dec-11 4:39am
v2

C++
for(ix = 0; ix < 5; ix ++)
{
          c cinst;
          cinst.SetI(ix);
          vctc.push_back(cinst);
}


First things first - all instances of the class object are being created within your for loop, and they will die IMMEDIATELY after the loop is done. Sure, you're creating 5 instances of the class, but the class objects (cinst) will NOT exist outside the for loop at all). Before asking anything else, you need to take care of that. How about putting those on the heap and deleteing them later?
 
Share this answer
 
Comments
Albert Holguin 7-Dec-11 10:48am    
Yeah, I don't think this is a valid test, good catch Rajesh. +5
JackDingler 7-Dec-11 10:49am    
Actually it will copy the class using the default copy constructor. So there will still be a class inside the vector.

The problem that will present though, is that this code allocates memory and needs a copy constructor.

By default, it will do a shallow copy which will result in a double delete of member pStr;

A deep copy needs to be implemented for this class.
Member 2246366 7-Dec-11 10:51am    
Thanks for the quick comment.
Of course cinst is gone after the for loop. But as far as I understand push_back it creates copies of the instance using the copy constructor.
Rajesh R Subramanian 7-Dec-11 11:03am    
OK, I kinda overlooked that. But still see Jack's comment.
Albert Holguin 7-Dec-11 13:30pm    
Me too actually
When it deletes the first item, every item after that, then gets shifted down one step.

So when you erase index 0, the following indexes change.

[0] -> Erased
[1] -> [0]
[2] -> [1]
[3] -> [2]
[4] -> [3]

So as you can see, the element at [4] is now at [3]...

Behind the scenes, all of that memory is being physically copied.

So you'll get code to this effect working behind the scenes.

vctc[0] = vctc[1];
vctc[1] = vctc[2];
vctc[2] = vctc[3];
vctc[3] = vctc[4];


So if you have large arrays, it's more efficient to use pointers to your object in the array, than the objects themselves.

************************

As I mentioned in the above solution, you need a deep copy to avoid double deletes.

Put this in your class definition.
C++
public:
    c(const c & rhs);


Then this in your .cpp
C++
c::c(const c & rhs)
{
    pStr = 0;
    SetI(rhs.i);
}
 
Share this answer
 
v4
Comments
Member 2246366 7-Dec-11 10:59am    
Hi Jack,

that's how I supposed it would work. But erase really deletes the 5th element. The instances' member strings contain "1", "2", "3" and something bad.

Cheers,

Thomas
JackDingler 7-Dec-11 11:01am    
Actually it now contains 0, 1, 2, and 3.

#4 is now undefined and you shouldn't try to access it.

Everything just gets shifted over one. Vectors always start at element [0].
Member 2246366 7-Dec-11 11:06am    
ok, but to write it clear
I get
vctc[0].pStr is "1"
vctc[1].pStr is "2"
vctc[2].pStr is "3"
vctc[3].pStr is undefined, deleted ...
Member 2246366 7-Dec-11 11:01am    
isn't this the same:
c(const c & other)
{
pStr = 0;
SetI(other.i);
}
JackDingler 7-Dec-11 11:03am    
Yes. Ooops, I skipped right over that. :)
This is your problem:
C++
char * pShouldBeDeleted = vctc[0].pStr;
char * pShouldNotBeDeleted = vctc[4].pStr;
vctc.erase(vctc.begin());

Following the call to erase() you have only 4 elements in the vector so the pointer pShouldNotBeDeleted is no longer valid as it points to vctc[4] which no longer exists (as Jack mentioned, they have all moved up one). If you step through your code with the debugger you will get a better idea of what is happening.
 
Share this answer
 
Comments
Member 2246366 7-Dec-11 11:39am    
replaced the last lines:
vctc.erase(vctc.begin());
char * pShouldNotHaveBeenDeleted = vctc[3].pStr;
still the same: vctc[3].pStr is somewhere in nirvana
It really has been deleted!

Forgot to mention that the app crashes when it leaves TestVectorErase(). The vector tries to delete it's items and fails to delete the last item
Richard MacCutchan 7-Dec-11 11:49am    
As I said before, use your debugger to step through the code and see exactly what is happening. Forget all these pointers, they are too volatile to be relied on after the vector has changed.
OK, I found another subtle issue that could probably solve your problem. You're having this char * pStr in your class that you're allocating memory with a new[10]; but unfortunately you're using delete instead of delete[] and that's screwing up things unpredictably.

When I removed that whole variable from your class, everything works as expected. I'd presume that if you want to use that char* variable, you must de-allocate the memory properly using delete[] instead of delete.
 
Share this answer
 
v3
Comments
JackDingler 7-Dec-11 11:47am    
The delete[] has been deprecated. It's no longer needed.
Rajesh R Subramanian 7-Dec-11 11:50am    
What? You mean, like I can use delete instead of delete[] even if I allocated multiple elements with new[]?
JackDingler 7-Dec-11 11:51am    
Yup. It's the new way.
JackDingler 7-Dec-11 12:00pm    
I take that back.

It's not working in Visual Studio 2008.

I'll try it in 2010 later.
Rajesh R Subramanian 7-Dec-11 12:26pm    
Well, I don't think it will work in VS 2010 either because it has nothing to do with the IDE - it's a mandate by the programming langauge. See this link: http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.12

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900