Proper way to reassign pointers in c++

Question

EDIT: I know in this case, if it were an actual class i would be better off not putting the string on the heap. However, this is just a sample code to make sure i understand the theory. The actual code is going to be a red black tree, with all the nodes stored on the heap.

I want to make sure i have these basic ideas correct before moving on (I am coming from a Java/Python background). I have been searching the net, but haven't found a concrete answer to this question yet.

When you reassign a pointer to a new object, do you have to call delete on the old object first to avoid a memory leak? My intuition is telling me yes, but i want a concrete answer before moving on.

For example, let say you had a class that stored a pointer to a string

class MyClass
{
private:
    std::string *str;

public:
MyClass (const std::string &_str)
{
    str=new std::string(_str);
}

void ChangeString(const std::string &_str)
{
    // I am wondering if this is correct?
    delete str;
    str = new std::string(_str)

    /*
     * or could you simply do it like:
     * str = _str;
     */ 
}
....

In the ChangeString method, which would be correct?

I think i am getting hung up on if you dont use the new keyword for the second way, it will still compile and run like you expected. Does this just overwrite the data that this pointer points to? Or does it do something else?

Any advice would be greatly appricated :D

Answer 1

Three comments:

You need a destructor as well.

~MyClass() 
{
    delete str;
}

---

You really don't need to use heap allocated memory in this case. You could do the following:

class MyClass {
    private: 
        std::string str;

    public:
        MyClass (const std::string &_str) {
            str= _str;
        }

        void ChangeString(const std::string &_str) {
            str = _str;
};

---

You can't do the commented out version. That would be a memory leak. Java takes care of that because it has garbage collection. C++ does not have that feature.

Answer 2

Why do you think you need to store a pointer to a string in your class? Pointers to C++ collections such as string are actually very rarely necessary. Your class should almost certainly look like:

class MyClass
{
private:
    std::string str;

public:
MyClass (const std::string & astr) : str( astr )
{
}

void ChangeString(const std::string & astr)
{
    str = astr;
}
....
};

Answer 3

When you reassign a pointer to a new object, do you have to call delete on the old object first to avoid a memory leak? My intuition is telling me yes, but i want a concrete answer before moving on.

Yes. If it's a raw pointer, you must delete the old object first.

There are smart pointer classes that will do this for you when you assign a new value.

Answer 4

Just pinpointing here, but

str = _str;

would not compile (you're trying to assign _str, which is the value of a string passed by reference, to str, which is the address of a string). If you wanted to do that, you would write :

str = &_str;

(and you would have to change either _str or str so that the constnest matches).

But then, as your intuition told you, you would have leaked the memory of whatever string object was already pointed to by str.

As pointed earlier, when you add a variable to a class in C++, you must think of whether the variable is owned by the object, or by something else.

If it is owned by the object, than you're probably better off with storing it as a value, and copying stuff around (but then you need to make sure that copies don't happen in your back).

It is is not owned, then you can store it as a pointer, and you don't necessarily need to copy things all the time.

Other people will explain this better than me, because I am not really confortable with it. What I end up doing a lot is writing code like this :

class Foo {

private :
   Bar & dep_bar_;
   Baz & dep_baz_;

   Bing * p_bing_;

public:
   Foo(Bar & dep_bar, Baz & dep_baz) : dep_bar_(dep_bar), dep_baz_(dep_baz) {
       p_bing = new Bing(...);
   }

   ~Foo() {
     delete p_bing;
   }

That is, if an object depends on something in the 'Java' / 'Ioc' sense (the objects exists elsewhere, you're not creating it, and you only wants to call method on it), I would store the dependency as a reference, using dep_xxxx.

If I create the object, I would use a pointer, with a p_ prefix.

This is just to make the code more "immediate". Not sure it helps.

Just my 2c.

Good luck with the memory mgt, you're right that it is the tricky part comming from Java ; don't write code until you're confortable, or you're going to spend hours chasing segaults.

Hoping this helps !

Answer 5

The general rule in C++ is that for every object created with "new" there must be a "delete". Making sure that always happens in the hard part ;) Modern C++ programmers avoid creating memory on the heap (i.e. with "new") like the plague and use stack objects instead. Really consider whether you need to be using "new" in your code. It's rarely needed.

If you're coming from a background with garbage collected languages and find yourself really needing to use heap memory, I suggest using the boost shared pointers. You use them like this:

#include <boost/shared_ptr.hpp>
...
boost::shared_ptr<MyClass> myPointer = boost::shared_ptr<MyClass>(new MyClass());

myPointer has pretty much the same language semantics as a regular pointer, but shared_ptr uses reference counting to determine when delete the object it's referencing. It's basically do it yourself garbage collection. The docs are here: http://www.boost.org/doc/libs/1_42_0/libs/smart_ptr/smart_ptr.htm

Answer 6

If you must deallocate the old instance and create another one, you should first make sure that creating the new object succeeds:

void reset(const std::string& str)
{
    std::string* tmp = new std::string(str);
    delete m_str;
    m_str = tmp;
}

If you call delete first, and then creating a new one throws an exception, then the class instance will be left with a dangling pointer. E.g, your destructor might end up attempting to delete the pointer again (undefined behavior).

You could also avoid that by setting the pointer to NULL in-between, but the above way is still better: if resetting fails, the object will keep its original value.

---

As to the question in the code comment.

*str = _str;

This would be the correct thing to do. It is normal string assignment.

str = &_str;

This would be assigning pointers and completely wrong. You would leak the string instance previously pointed to by str. Even worse, it is quite likely that the string passed to the function isn't allocated with new in the first place (you shouldn't be mixing pointers to dynamically allocated and automatic objects). Furthermore, you might be storing the address of a string object whose lifetime ends with the function call (if the const reference is bound to a temporary).

Answer 7

I'll just write a class for you.

class A
{
     Foo * foo;   // private by default
 public:
     A(Foo * foo_): foo(foo_) {}
     A(): foo(0) {}   // in case you need a no-arguments ("default") constructor
     A(const A &a):foo(new Foo(a.foo)) {}   // this is tricky; explanation below
     A& operator=(const &A a) { foo = new Foo(a.foo); return *this; }
     void setFoo(Foo * foo_) { delete foo; foo = foo_; }
     ~A() { delete foo; }
}

For classes that hold resources like this, the copy constructor, assignment operator, and destructor are all necessary. The tricky part of the copy constructor and assignment operator is that you need to delete each Foo precisely once. If the copy constructor initializer had said :foo(a.foo), then that particular Foo would be deleted once when the object being initialized was destroyed and once when the object being initialized from (a) was destroyed.

The class, the way I've written it, needs to be documented as taking ownership of the Foo pointer it's being passed, because Foo * f = new Foo(); A a(f); delete f; will also cause double deletion.

Another way to do that would be to use Boost's smart pointers (which were the core of the next standard's smart pointers) and have boost::shared_ptr<Foo> foo; instead of Foo * f; in the class definition. In that case, the copy constructor should be A(const A &a):foo(a.foo) {}, since the smart pointer will take care of deleting the Foo when all the copies of the shared pointer pointing at it are destroyed. (There's problems you can get into here, too, particularly if you mix shared_ptr<>s with any other form of pointer, but if you stick to shared_ptr<> throughout you should be OK.)

Note: I'm writing this without running it through a compiler. I'm aiming for accuracy and good style (such as the use of initializers in constructors). If somebody finds a problem, please comment.