-2

I would like to know if copying an object in the following manner is acceptable vis-a-vis copying the individual elements.

#include <iostream>
using namespace std;

class abc{
public:
    abc(int a = 10, int b = 5);
    abc(const abc &obj, int b = 10);

    int ret_x() { return x; }
    int ret_y() { return y; }
private:
    int x;
    int y;
};

abc::abc(int a, int b)
    : x(a),
    y(b)
{
}

abc::abc(const abc &obj, int b)
{
    if (this != &obj) {
        *this = obj;    -----> Copying the object
    }
    y = b;
}

int main()
{
    abc a1;
    cout << "A1 values: " << a1.ret_x() << "\t" << a1.ret_y() << endl;
    abc a2(a1, 20);
    cout << "A2 values: " << a2.ret_x() << "\t" << a2.ret_y() << endl;

    return 0;
}

Edit:

Use case:

The issue is that object a1 is auto-generated and hence any newly introduced members in the class could not be updated. I could provide a member function to update the new members, sure, but wanted to explore this option.

The code works fine, but is the method correct?

Thanks!

Maddy
  • 1,141
  • 1
  • 18
  • 33
  • You're constructing a new object - when would it make sense for a new object to be created from itself? – chris Jul 15 '16 at 05:38
  • i haven't see constructor like this. Copy constructor is copy from others. But you include your special intialize of one member variable, this code break the common sense of copy constructor. Why the constructor initial b 5 but copy constructor initial b 0? – Tongxuan Liu Jul 15 '16 at 07:20
  • @TongxuanLiu: Added use case – Maddy Jul 15 '16 at 08:45
  • 1
    @Maddy At a first glance, the member function option appears supperior to me - you do not need to create a copy of a1, but can operate on it directly... – Aconcagua Jul 15 '16 at 09:39

1 Answers1

2

As chris noted already in the comments, you are creating a completely new object. How would you want to get this passed into the constructor? Well, actually, you could perhaps via placement new:

abc a;
abc* b = new(&a)abc(a);

But this is a such an exotic case that I would not consider it, I even dare to claim someone using advanced stuff such as placement new should know what he is doing... So leave out the if-check.

In your special case, it seems OK, as no data exists that might require deep copying. Be aware, though, that you are assigning the member b twice. Not really critical with int, but on larger objects (std::string, std::vector, ...) which do deep copies this gets more and more questionable.

With C++11, though, I would prefer constructor delegation:

abc::abc(const abc& obj, int b)
    : abc(obj) // copying here
{
    y = b;
}

This does not solve, however, the double assignment problem. To be honest, this might not always be a true problem, in many cases the compiler might optimise the first assignment away (especially in the int case of our example). But on more complex data types (possibly already std::string), I wouldn't feel comfortable relying on the compiler detecting obsolete assignment...

Be aware that you might get into trouble if you have resources managed internally:

struct abc
{
    int* array;
    abc() : array(new int[7]) { }
    ~abc()
    {
        delete[] array;
    }
}

Not providing an appropriate assignment operator or copy constructor – depending on the implementation variant, yours (assignment) or mine (constructor delegation) – doing the necessary deep copy will result in multiple deletion of the same data (undefined behaviour!). Following the rule of three (or more recently, rule of five), you most probably will need both anyway. You might consider the copy and swap idiom idiom then.

Finally a trick to avoid double assignment:

abc(abc const& other)
    : abc(other, other.y)
{ }

abc(abc const& other, int y)
    : x(other.x), y(y)
{ }
Community
  • 1
  • 1
Aconcagua
  • 19,952
  • 4
  • 31
  • 51
  • Glad you mentioned about [Rule of three](https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)) while answering it. – ani627 Jul 15 '16 at 07:06
  • @Aconcagua: Added the use case for doing things this way. – Maddy Jul 15 '16 at 08:51