2

Assuming the constructor, destructor, and assignment operator is written properly, why can't I implement the copy-constructor like this:

MyClass::MyClass(const MyClass &other)
{
    this->value = new Value(*(other.value));
}

Most of the examples I see, do this: (because they're dealing with arrays usually)

MyClass::MyClass(const MyClass &other)
{
    Value *temp = new Value;
    *temp = *(other.value);
    this->value = temp;
}

But in the first example, if 'new' throws, 'other' isn't affected, and if Value's copy-constructor throws, won't 'new' free the allocated memory before propogating the exception?

As this is for a mini-smart pointer itself, I'm specifically avoiding the use of std::unique_ptr and other smart pointers.

Jamin Grey
  • 8,807
  • 4
  • 35
  • 48
  • Why is `value` dynamically allocated in the first place? You're trying to give it value semantics, at a high cost, and doing it wrong as well? – sehe Mar 18 '13 at 02:17
  • Value is a pointer to another class. Perhaps the name 'Value' was the wrong word - I should've used the more generic 'Data', or 'OtherClass'. And if I'm, "Doing it wrong as well", that's why I'm asking the question, because I don't know if I'm doing it right or not. – Jamin Grey Mar 18 '13 at 02:22
  • So, why is it a pointer? If copying `MyClass` means copying `value`, that's simply value aggregation. – sehe Mar 18 '13 at 02:23
  • It's an implementation of a smart pointer partly as a learning exercise and partly as a helper class for a specific purpose. My code normally uses std::unique_ptr and std::shared_ptr for classes, but in this situation I want my own implementation. – Jamin Grey Mar 18 '13 at 02:27
  • 2
    @JaminGrey There's nothing wrong with implementing the copy constructor as you've shown. If the copy constructor throws, [the memory allocated by new will be freed before the exception propagates](http://stackoverflow.com/questions/4094996/what-happens-to-the-memory-allocated-by-new-if-the-constructor-throws). The second example, in fact, might leave you with leaked memory if Value's assignment operator throws. – Praetorian Mar 18 '13 at 02:28
  • Thank's Praetorian for answering my question! You should make it an Answer so I can mark it properly. =) – Jamin Grey Mar 18 '13 at 02:29
  • If you have to keep `this->value` as a pointer member, then your construction should at least utilize the copy-ctor of the Value class on allocation, rather than the assignment operator post-allocation. IOW, I prefer the first method, but alas, both have potential issues. – WhozCraig Mar 18 '13 at 02:29
  • @WhozCraig I'm not sure I understand. Isn't that exactly what I'm doing in the first example? Could you explain further? – Jamin Grey Mar 18 '13 at 02:37
  • Ah, my mistake, I missed the "IOW, I prefer the first method". What potential issues does the first version have? – Jamin Grey Mar 18 '13 at 02:51
  • 1
    @WhozCraig Isn't it cleaned up by 'new'? That was what the [link](http://stackoverflow.com/questions/4094996/what-happens-to-the-memory-allocated-by-new-if-the-constructor-throws) Praetorian posted implied, and what [@bames53's answer](http://stackoverflow.com/a/15468877/1177073) says, unless I'm misreading things, which I probably am. Won't **new** catch an exception thrown within the constructor or copy contructor of a class it is in the process of allocating? – Jamin Grey Mar 18 '13 at 02:59
  • 1
    @JaminGrey yes, you are entirely correct, I had to go hobble through posts and a few standard references to know for sure, but he (Paretorian) and you are quite correct; that is the way it should behave, and therefore I'm solidly in the camp of Option #1 =P – WhozCraig Mar 18 '13 at 07:38

2 Answers2

2

and if Value's copy-constructor throws, won't 'new' free the allocated memory before propogating the exception?

Yes.

There's no particular reason not to use the single line method instead of the three line assignment version.

Since you're writing a smart pointer the following doesn't apply, but in normal classes you would probably wrap the manual pointer management up into a RAII type. It looks like std::unique_ptr has the semantics you want, and a make_unique helper makes it pretty easy:

#include <memory>

// probably will be added to the standard
template<typename T, typename... Args>
std::unique_ptr<T> make_unique(Args &&... args) {
    return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}

class Value {};

class MyClass {
    std::unique_ptr<Value> value;

public:
    MyClass() : value{make_unique<Value>()} {}

    MyClass(MyClass const &other) : value{make_unique<Value>(*other.value)} {}

    MyClass &operator= (MyClass const &other) {
        value = make_unique<Value>(*other.value);
        return *this;
    }
    // you can also implement assignment from rvalue ref as an optimization
};
bames53
  • 79,748
  • 13
  • 162
  • 229
  • Yep, it's because 'other' is const that I said it wouldn't be affected - that part wasn't a question but a statement. =) Thanks for answering the actual question also! I do already have a make_unique helper, and use std::unique_ptr and std::shared_ptr normally in my code, I should've mentioned I was specifically avoiding it for this specific class. Oops. – Jamin Grey Mar 18 '13 at 02:33
1

There are too many problems to start enumerating. I recommend you learn the Rule Of Three:

Like I said, the proper solution here would likely read

struct MyClass
{
     MyClass(const MyClass &other) : value(other.value) {}

    private:
     OtherClass value;
};

And if value is some kind of resource that must live on the heap, it would then be declared as

struct MyClass
{
    // ...
    private:
       std::unique_ptr<OtherClass> value;
};

That way you can't (well, easily) go wrong with ownership semantics and memory management.

Community
  • 1
  • 1
sehe
  • 328,274
  • 43
  • 416
  • 565
  • I'm asking a question about part of the rule of three that I didn't understand. I mentioned that in the original question. Offering std::unique_ptr is a good suggestion though! But I'm specifically avoiding it in this situation. – Jamin Grey Mar 18 '13 at 02:30