CODEWITHSUNDEEP
c++pointersvectordestructordelete-operator
Deukalion
Deukalion

Reputation: 2655

Delete pointer to vector of char* in destructor (Not working)

I have a class that holds a few vectors, I'm not sure which method is the best but when the I call the destructor they should be deleted from memory.

HEADER:

class Test
{
public:
    Test();
    ~Test();

    void AddString(char* text);
    void AddString(string text);

private:
    char * StringToCharPointer(string value);

    vector<char*> *pVector;
}

CPP File:

Test::Test()
{
};

Test::~Test()
{
    vector<char*>::iterator i;

    for ( i = pVector->begin() ; i < pVector->end(); i++ )
    {
        delete * i;
    }

    delete pVector;
};

char * Test::StringToCharPointer(string value)
{
    char *pChar = new char[value.length()];
    strcpy(pChar, value.c_str());

    return pChar;
};

Test::AddString(char* text)
{
    pVector->push_back(text);
};

Test::AddString(string text)
{
    pVector->push_back(StringToCharPointer(text));
};

so here's pretty much all the methods that I use, but what's wrong?

Upvotes: 0

Views: 1882

Answers (4)

Steve Jessop
Steve Jessop

Reputation: 279225

Firstly, i is an iterator on the vector, it is not the pointer stored in the vector. *i is the pointer stored in the vector, so if you're going to delete anything it should be that.

Secondly, delete *i is only valid if the object pointed to by *i was allocated with new. Not new[], not malloc, and it doesn't point to a string literal. Since you don't say how your data was allocated, it is not possible for us to say whether or not you are freeing it correctly.

It seems likely that you should use a std::vector<std::string>.

Update for updated question:

HEADER:

class Test
{
public:
    Test();
    ~Test();

    void AddString(const string &text);
private:
    vector<string> mVector;
};

CPP file:

Test::Test()
{
};

Test::~Test()
{
};

void Test::AddString(const string &text)
{
    mVector.push_back(text);
};

Upvotes: 7

Jonas Bystr&#246;m
Jonas Bystr&#246;m

Reputation: 26129

It seems likely that you should use a std::vector<std::string>, to shorten the wise words of Steve Jessop.

To elaborate a little more: you say you want "to make the memory allocation smaller", but sounds like you're at the wrong path if you don't know pointers, and correct me if I'm wrong in guessing premature optimization (usually the case with inexperienced developers in this type of question).

Upvotes: 0

Naveen
Naveen

Reputation: 73433

This is one obvious problem: char *pChar = new char[value.length()];. You are doing new[] but doing delete in destructor which invoked undefined behavior. You should use delete[] to delete those pointers. But using delete[] might give problems for Test::AddString(char* text) method as you can not be sure how memory for text is allocated i.e. using new or new[] or malloc. The simplest way is to use std::vector<std::string> as suggested by Steve Jossep.

Upvotes: 0

jpalecek
jpalecek

Reputation: 47762

Your destruction code looks fine (although I guess you meant delete *i; in the second snippet, since otherwise, ti wouldn't have even compiled.

However, the errors you are getting indicate you put bad things in your vectors. The only char*s that can be inserted in the vector with such destruction code are the ones returned by new char. Especially, you must not insert literals ("abc") or strings that are made as parts of other strings (strtok(NULL, ":"), strchr(str, ':') into it.

Upvotes: 0

Related Questions