1

For my home work I need to write String object in C++.

One of the methods is

void concatenate(String *s)

But when I check my object with Valgrind, there is a memory leak in my code. Here is method:

// add s's str to this _str
void String::concatenate(String *s)
{
  char * conc;
  int conc_size, i, j;

  conc_size = _len + s->_len;

  conc = new char[conc_size];  // line 39

  for (i = 0; i < _len; i++)
    conc[i] = _str[i];

  for (j = 0; i < conc_size || j < s->_len; i++, j++)
    conc[i] = s->_str[j];

  _str = conc;   // i'm assuming the problem is here
  _len = conc_size;
}

Here is Valgrind message:

==4706== 3 bytes in 1 blocks are definitely lost in loss record 2 of 9
==4706==    at 0x100024679: malloc (vg_replace_malloc.c:266)
==4706==    by 0x10007BF04: operator new(unsigned long) (in /usr/lib/libstdc++.6.0.9.dylib)
==4706==    by 0x10007BF96: operator new[](unsigned long) (in /usr/lib/libstdc++.6.0.9.dylib)
==4706==    by 0x100000E7E: String::concatenate(String*) (String.cpp:39)
==4706==    by 0x100001479: main (main.cpp:27)

Works with delete [] _str; and changed constructors:

// the empty string.
String::String()
{
    _len = 1;

    _str = new char[_len];
    assert(_str);
    _str[0] = '\0';
}

// store s string in this
String::String(char *s)
{
    _len = 0;

    int i = 0;
    while(s[i] != '\0')
    {
        _len++;
        i++;
    }

    _str = new char[_len];
    assert(_str);

    for (i = 0; s[i] != '\0' && i < _len; i++)
        _str[i] = s[i];

}
4
  • You are right. You forgot to delete [] _str; before assigning conc to it
    – Lol4t0
    Feb 11, 2012 at 19:26
  • Who gives homework to implement something which is already aptly solved with std::string? Feb 11, 2012 at 19:26
  • Unrelated side note: shouldn't the condition in the second loop be i < conc_size && j < s->_len? Besides, why don't you just use std::copy?
    – reima
    Feb 11, 2012 at 19:30
  • @JohnLeidegren: my teacher :) Feb 11, 2012 at 19:43

6 Answers 6

4

You're not dealocating the previous contents of your member _str. I'm assuming that's also a char*. So when you're telling _str to point elsewhere, the memory it previously pointed to will be unacessible.

delete[] _str;
_str = conc;
6
  • @user1090944 In another answer, it's suggested that you check _str != NULL before you call delete. That's bad advice, as delete on a null pointer is a no-op. Feb 11, 2012 at 19:29
  • I did that, but Valgrind don't like it: Feb 11, 2012 at 19:30
  • ==4957== Invalid free() / delete / delete[] / realloc() Feb 11, 2012 at 19:32
  • @user1090944 so the problem persists even with the delete? Can you post the new code, including the class declaration? Feb 11, 2012 at 19:32
  • @user1090944 What was your problem exactly? Did you forget to initialize _str to a sensible value in all your constructors?
    – reima
    Feb 11, 2012 at 20:40
1

Before line

_str = conc;

Put

 delete[] _str;
2
  • delete is guaranteed to be safe on a null pointer, so the if (_str) is unnecessary
    – Zrax
    Feb 11, 2012 at 19:28
  • Well, you are right. It's possible to omit this checking. Corrected. Feb 11, 2012 at 19:28
1

You're right, you are overwriting your internal pointer with a new one without deleting it.

1

Why don't you delete[] _str? This is dangling now.

0

Your assumption is about right: You assign a new value to the _str pointer, but the memory that is allocated for it remains allocated. You need to call delete[] _str; right before that line to free the memory.

0

I assume the String::_str is the char * hold the main pointer to data.

Therefore, you must consider an array delete to _str before you assign the conc.

delete[] _str;
_str = conc;

Addition, it's a good practice for you to consider the argument of the concatenate method like concatenate(const String &).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.