CODEWITHSUNDEEP
c++pointers
user1861088
user1861088

Reputation: 1733

C++ invalid pointer error

I'm getting invalid point error from the code below I don't see why. All I'm trying to do is to delete free some strings on the heap from a vector:

void func() {
    vector<string>* vec = new vector<string>;
    vec->push_back(*(new string("1")));
    vec->push_back(*(new string("2")));

    for(vector<string>::iterator itr = vec->begin(); itr != vec->end(); ++itr)
    {
        string* ptr = &(*itr);
        delete(ptr);
    }
}

EDIT: is it because push_back creates a copy of the string?

Upvotes: 2

Views: 18978

Answers (3)

WhozCraig
WhozCraig

Reputation: 66254

Your error is because the element isn't dynamically allocated; the vector is. What you're trying to do would require:

void func() 
{
    vector<string*> vec;
    vec.push_back(new string("1"));
    vec.push_back(new string("2"));

    for(vector<string*>::iterator itr = vec.begin(); itr != vec.end(); ++itr)
    {
        string* ptr = *itr;
        delete(ptr);
    }
}

But honestly I see little reason to do this. As-written your code not only attempts to delete memory it never actually allocated, it leaks what it allocations it did make.

There are reasons to store pointers to objects in a vector like this (such as the objects being actually from another container somewhere else and you need a temporary list of them for a custom-sort operation without disturbing the original content), but something tells me you're a ways-off from having such a need.

Upvotes: 6

Charles Salvia
Charles Salvia

Reputation: 53339

Firstly, the line

vec->push_back(*(new string("1")));

is causing a memory leak. The value returned from new string("1") is a pointer to a newly allocated string object. But when you dereference it and insert it into the vector, a copy of the heap-allocated object is created and inserted. However, the actual string object you originally allocated on the heap is leaked.

Essentially, your vector is storing string objects by value, not pointers to string objects. The copy of the string object that is inserted into the vector is NOT a heap allocated object (not an object allocated with new). And of course, you cannot delete something that wasn't allocated with new. So when you call delete(ptr) you are causing undefined behavior.

What you seem to want here is a:

vector<string*>* vec = new vector<string*>;

However, in general I don't see any compelling reason why you are allocating everything on the heap. In C++ it is preferable to use stack allocation and containers with value semantics whenever feasible, unless you have some reason why you need heap allocation (e.g. a container of polymorphic objects, in which case you should use smart pointers anyway). Generally, when new C++ programmers use heap-allocated objects and the new keyword all over the place, it is a sign that they are poorly transliterating a programming style imported from a managed language like Java or C#.

Upvotes: 4

Nik Bougalis
Nik Bougalis

Reputation: 10613

No you're not - your vector stores string objects, not pointers to string objects. That's why you have the * in your push_back call - you are dereferencing the returned pointer.

You are adding a copy of the dynamic string you create with new and that dynamic string is lost, since you never store the pointer that new returns.

Upvotes: 3

Related Questions