Is it a good practice to point to a new address on free store when dynamicly allocating?

Question

Below is an exercise from C++ Primer 5th Edition:

Exercise 13.22: Assume that we want HasPtr to behave like a value. That is, each object should have its own copy of the string to which the objects point. We’ll show the definitions of the copy-control members in the next section. However, you already know everything you need to know to implement these members. Write the HasPtr copy constructor and copy-assignment operator before reading on.(Page 511)

Codes for Class HasPtr:

class HasPtr
{
public:
    //! default constructor
    HasPtr(const std::string &s = std::string()):
        ps(new std::string(s)), i(0) { }

    //! copy constructor
    HasPtr(const HasPtr& hp) : ps(new std::string(*hp.ps)), i(hp.i) { }

    HasPtr&
    operator = (const HasPtr& hp);

    ~HasPtr()
    {
        delete ps;
    }

private:
    std::string *ps;
    int    i;
};

My code for this copy-assignment operator:

HasPtr&
HasPtr::operator = (const HasPtr &hp)
{
    delete ps;
    ps = new std::string(*hp.ps);

    i  = hp.i;

    return *this;
}

Codes presented in the following section from this book:

HasPtr& 
HasPtr::operator = (const HasPtr &rhs)
{
    auto newp = new string(*rhs.ps);   // copy the underlying string
    delete ps;       // free the old memory
    ps = newp;       // copy data from rhs into this object
    i = rhs.i;
    return *this;    // return this object
}

By executing step by step, I found a slight difference between the two codes. In my code it doesn't change the address to which ps points, whereas the code from the book makes ps point to a new address. I'm wondering whether this subtle difference has any significant meaning? Should I always change the pointer to a new address in similar situations? why?

copy-swap is good because its efficient in the general case, and tends to avoid self assignment and exception safety issues that many such pieces of code have. — RichardPlunkett, Jan 01 '14 at 01:58
It's not at all clear why the string is a pointer? The contents of the string are already stored on the heap. There is no performance hit to storing it locally if you take advantage of move semantics. If you are inclined to keep it as a pointer, consider using a smart pointer. This code is not exception-safe. I too suggest reading up on the copy-swap idiom. — Mark, Jan 01 '14 at 01:59
True, I'm guessing this code is for demonstration purposes on how to make an RAII class, but if you kill the pointers, you don't need the destructor, copy constructor, or copy assignment operator. — chris, Jan 01 '14 at 02:01

score 2 · Accepted Answer · answered Jan 01 '14 at 02:07

Your code has a problem with self-assignment and with exceptions: assume that the memory allocation throws a std::bad_alloc exception. When coding, you should always assume that memory allocations can go wrong although the actually rarely do. In the code

delete ps;
ps = new std::string(*hp.ps);

ps would point to stale member when the second line of code throws an exception. Incidentally, if you end up self-assigning the object, you actually delete the memory of the only before accessing it. Thus, it is a good idea to first copy the content of the right hand side, then put things into place, and finally release resource.

As it happens, these are exactly the operations of

the copy constructor
a swap() operation you generally want for any type holding resources
the destructor

The way to leverage these three operation is known as copy and swap idiom:

T& T::operator=(T other) {
    this->swap(other);
    return *this;
}

Note the signature change. This particular code has used pass-by-value to do the copy construction. Doing that is optional, vs doing a copy construction inside the operator, but the code is smaller and cleaner this way. — RichardPlunkett, Jan 01 '14 at 08:22

RichardPlunkett · Answer 2 · 2014-01-01T02:09:57.120

Your version is unsafe for self assignment.

delete ps;
ps = new std::string(*hp.ps);

Here,if doing a self assignment, you may be deleting the ps in both the source and destination, making its use in the new statement wrong (tho it may deceptively work on most occasions).

You can assign the value of the new directly into your member variable, but you can't just arbitrarily delete ps, before knowing if you need it.

If you tested for self assignment eg.if (this!=&hp) before doing your code, then it would be better, but still not ideal (see exception safety comments elsewhere).

score 1 · Answer 3 · answered Jan 01 '14 at 02:09

Functionally I can only see one difference.

    delete ps;
    ps = new std::string(*hp.ps);

If memory is low, the call to new std::string might possibly throw an exception. In your case, ps still has the address of the old deleted string -- so it is malformed. If you recover from the exception somebody might dereference ps and bad things will happen.

    auto newp = new string(*rhs.ps);   // copy the underlying string
    delete ps;       // free the old memory
    ps = newp;       // copy data from rhs into this object

In the text book code, ps is not deleted until after the new string is allocated. On exception, ps still points to a valid string so you do not have a malformed object.

How much of a problem that is depends on a few different things, but it is generally better practice to avoid any chance of a malformed object.

score 1 · Answer 4 · answered Jan 01 '14 at 02:20

There are actually two issues in your code:

Self-assignment
Exception safety

You normally want your member functions to provide strong exception guarantee, i.e., when assignment fails (in your case, it can be in operator new or the copy-constructor of string), the program state does not change.

I think a modern practice is to provide a swap function and make the assignment call the copy constructor. Something like:

void HasPtr::swap(HasPtr& rhs)
{
    std::swap(this.ps, rhs.ps);
    std::swap(this.i, rhs.i);
}

HasPtr(const HasPtr& rhs)
{
    ps = new string(*rhs.ps);
    i = rhs.i;
}

HasPtr& operator=(const HasPtr& rhs)
{
    HasPtr temp(rhs);
    this.swap(temp);
    return *this;
}

Is it a good practice to point to a new address on free store when dynamicly allocating?

4 Answers4