Why do I need to delete resources when using copy-assign operator?
Asked Answered
E

2

5

Code like this from one of my books for example:

class HasPtr {
public:
    HasPtr(const HasPtr& h): ps(new std::string(*h.ps)), i(h.i) { }
    HasPtr(const std::string &s = std::string()): ps(new std::string(s)), i(0) { }
    HasPtr& operator=(const HasPtr&);
    ~HasPtr() { delete ps; }
private:
    std::string *ps;
    int i;
};

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
}

Seems like the inside of the operator= could just be:

*ps = *rhs.ps
i = rhs.i
return *this;

With no need to delete the pointer first, seems redundant to do so. It did mention it is written in a way to leave the object in a suitable state should an exception occur but didn't divulge past that, but I don't see what exception could occur that even my alternative wouldn't handle. Why is there a need to delete the object first before assigning?

Emolument answered 27/8, 2015 at 23:44 Comment(3)
You need to observe the rule of three (which is trivially observed by following the rule of zero). Also use copy-and-swap if you implement your own assignment operator.Duplication
@LightnessRacesinOrbit C++ Primer 5th Edition, Chapter 13.2.1Emolument
If you jump to page 644, they provide a fixed version of their assignment operator.Duplication
R
4

This looks fine to me.

And you're right, std::string assignment already offers a strong exception guarantee so you will still leave the object in its original state should an exception occur copying the string.

Of course there is no reason to allocate a std::string with new like that. You could just write this instead:

class HasNoPtr {
public:
    HasNoPtr(const std::string& s): ps(s), i(0) { }
private:
    std::string ps;
    int i;
};
Rallentando answered 28/8, 2015 at 0:19 Comment(0)
B
5

In this case, yes, that would be fine.

You're not leaking the dynamically-allocated string: you're re-using it.

Boastful answered 28/8, 2015 at 0:1 Comment(0)
R
4

This looks fine to me.

And you're right, std::string assignment already offers a strong exception guarantee so you will still leave the object in its original state should an exception occur copying the string.

Of course there is no reason to allocate a std::string with new like that. You could just write this instead:

class HasNoPtr {
public:
    HasNoPtr(const std::string& s): ps(s), i(0) { }
private:
    std::string ps;
    int i;
};
Rallentando answered 28/8, 2015 at 0:19 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.