Should I delete the pointer in this case?

Question

In my own string class(just for learning) i have overload of operator + Do I need to clean the memory pointed by tmpPtr?

String String::operator+(const String& rv) const
{
    size_t newLength = length + rv.length;
    char* tmpPtr = new char[newLength + 1];
    strcpy(tmpPtr, ptr);
    strcpy(tmpPtr + length, rv.ptr);
    String S(tmpPtr);
    delete[] tmpPtr;
    return S;
}

Can i change this method this way?

String String::operator+(const String& rv) const
{
    size_t newLength = length + rv.length;
    char* tmpPtr = new char[newLength + 1];
    strcpy(tmpPtr, ptr);
    strcpy(tmpPtr + length, rv.ptr);
    return S(tmpPtr);
}

If someone needs a constructor and a destructor:

String::String(const char* str)
{
    setString(str, strlen(str));
}

String::String(const String& str)
{
    setString(str.ptr, str.length);
}
void String::setString(const char* str, size_t size)
{
    ptr = new char[size + 1];
    strcpy(ptr, str);
    length = size;
}
String::~String()
{
    delete[] ptr;
}

That depends, can you show us the constructor for your String class? (I assume it's an exercise) — Benjamin Gruenbaum, Jun 01 '14 at 13:01
@EdHeal looking at the code, OP is probably learning c++ and is trying to implement a String class as an exercise. — Benjamin Gruenbaum, Jun 01 '14 at 13:04
Tip: Consider overloading `+=` as a member function, and use that to define `+` as a non-member. Also read: [Operator Overloading](http://stackoverflow.com/questions/4421706/operator-overloading) Also, this GOTW article should be interesting: [Monoliths "Unstrung"](http://www.gotw.ca/gotw/084.htm) — Deduplicator, Jun 01 '14 at 13:06
@OrangeFox in that case, yes, you should delete that string. Or better, don't perform two allocations. — Benjamin Gruenbaum, Jun 01 '14 at 13:12
@BenjaminGruenbaum _Or better, don't perform two allocations_.Please explain what you mean. — Orange Fox, Jun 01 '14 at 13:27

Cactus Golov · Answer 1 · 2014-06-01T13:33:29.767

As Vlad says, it depends on how you implement the constructor.

If your constructor simply takes ownership of the pointer and then deletes it in the destructor, you do not need a delete[] here. However, this is problematic, as it means that you cannot safely construct a String instance directly from a string literal.

Usually, a better way to write the constructor is to copy the contents of the passed-in pointer to a new block of memory and then store a pointer to that. This means that the memory usage of String is symmetric: it deallocates everything it allocates, and nothing else. This means that if you new[] something and then pass that pointer to your String's constructor, you still need to delete[] it.

However, whichever way it is, neither option is a particularly good fit here. Instead, you should do something like this:

String String::operator+(const String& rv) const
{
    size_t newLength = length + rv.length;
    // Create a string with enough backing storage
    String S(newLength + 1);
    strcpy(S.ptr, ptr);
    strcpy(S.Ptr + length, rv.ptr);
    return S;
}

This makes sure that it is the String instance managing the memory involved, so you get all the clean-up guarantees you normally get from a constructor/destructor pair.

Edit: With the body of the constructor available we can conclude that you took the second approach. Therefore, if you keep using this approach, you do need to delete[] the memory.

`String S(newLength + 1);`Do you want to say that I need to make a constructor that takes a string size? — Orange Fox, Jun 01 '14 at 13:24
@OrangeFox yes, that what he is saying. The constructor should allocate the backing storage and the destructor should release it. That way ownership is clear. — Benjamin Gruenbaum, Jun 01 '14 at 13:24
@OrangeFox: Yes, you should. An interesting question is whether you take the length or the length + 1. I'd do the former if your length constructor is going to be public. If it's an implementation detail, you're free to choose. — Cactus Golov, Jun 01 '14 at 13:35

score 1 · Answer 2 · edited Jun 20 '20 at 09:12

1

Yes. In general, you need to delete the memory allocated by new.

Wiki says:

In the C++ programming language, the delete operator calls the destructor of the given argument, and returns memory allocated by new back to the heap.[1] A call to delete must be made for every call to new to avoid a memory leak.

C++11: 3.8 Object lifetime (p4):

..... if there is no explicit call to the destructor or if a delete-expression (5.3.5) is not used to release the storage, the destructor shall not be implicitly called and any program that depends on the side effects produced by the destructor has undefined behavior.

EDIT: As per the OP's edit, as your destructor is deallocating the memory, in this case no need to delete the ptr explicitly. But still need to delete tmpPtr using delete[] operator.

edited Jun 20 '20 at 09:12

Community

1
1

answered Jun 01 '14 at 13:00

haccks

104,019
25
176
264

3

It's impossible to tell given the code OP has provided. – Benjamin Gruenbaum Jun 01 '14 at 13:01
1

this is a general guideline - whenever you define a `new` somewhere you should use a `delete` on the memory you've allocated. – NirMH Jun 01 '14 at 13:02
The String class might take ownership of it. It's not clear from the question. – isarandi Jun 01 '14 at 13:06
@isarandi; See the edited [question](http://stackoverflow.com/q/23980132/2455888). I am talking in general sense. – haccks Jun 01 '14 at 13:11

score 1 · Answer 3 · answered Jun 01 '14 at 13:29

Since your String::setString member function which is invoked by the constructor String::String creates its own char array in dynamic memory and copies the contents of the passed string there:

void String::setString(const char* str, size_t size)
{
    ptr = new char[size + 1];
    strcpy(ptr, str);
    length = size;
}

The originally created memory in String::operator+ is no longer required, is stored nowhere and will therefore leak if not deleted. So, yes, you have to delete[] it.

Should I delete the pointer in this case?

3 Answers3

C++11: 3.8 Object lifetime (p4):