2

Consider the below code, where a composing class with another class as its member is being instantiated:

class CopyAble {
private:
    int mem1;
public:
    CopyAble(int n1) : mem1(n1) {
        cout << "Inside the CopyAble constructor" << endl;
    }
    CopyAble(const CopyAble& obj) {
        cout << "Inside the CopyAble copy constructor" << endl;
        this->mem1 = obj.mem1;
        return *this;     
    }

    CopyAble& operator=(const CopyAble& obj) {
        cout << "Inside the  CopyAble assignment constructor" << endl;
        this->mem1 = obj.mem1;
    }
    ~CopyAble() {};
};

class CopyAbleComposer {
    private:
        CopyAble memObj;
    public:
        CopyAbleComposer(CopyAble m1) : memObj(m1) {
            cout << "Composing the composer" << endl;
        }
        ~CopyAbleComposer() {}
};

int main()
{
    CopyAble ca(10);
    CopyAbleComposer cac(ca);
    return 0;
}

When I run this, I get the output:

Inside the CopyAble constructor  
Inside the CopyAble copy constructor  
Inside the CopyAble copy constructor  
Composing the composer

Which means that the CopyAble copy constructor is being run twice - once when the CopyAble object is passed into the CopyAbleComposer constructor, and again when the initializer memObj(m1) runs.

Is this an idiomatic use of the copy constructor? It seems very inefficient that the copy constructor runs twice when we try to initialize a member object with a passed-in object of the same type, and it seems like a trap a lot of C++ programmers can easily fall into without realizing it.

EDIT: I don't think this is a duplicate of the question regarding passing a reference into the copy constructor. Here, we are being forced to pass a reference into a regular constructor to avoid duplicate object creation, my question was that is this generally known that class constructors in C++ should have objects passed in by reference to avoid this kind of duplicate copy?

SR Bhaskar
  • 898
  • 7
  • 17
  • Change `CopyAbleComposer` ctor to `CopyAbleComposer(const CopyAble& m1)` to avoid extry copying. – Paul Nov 13 '17 at 16:44
  • 2
    A setter doesn't change anything, you still have to pass a value to a function. *How* you pass it is more important. – Bo Persson Nov 13 '17 at 16:46
  • @Paul -- is this commonly known to experienced C++ programmers that one should use pass-by-reference when initializing object members while constructing an object? – SR Bhaskar Nov 13 '17 at 16:49
  • The method causing the copies isn't the copy constructor, but rather the (converting) constructor of `CopyAbleComposer` from `CopyAble`. – underscore_d Nov 13 '17 at 16:51
  • 2
    Passing a parameter by value means you *want* a copy. – n. m. could be an AI Nov 13 '17 at 16:52
  • 3
    When you don't want to copy, pass by reference, that's pretty commonly known. Doesn't matter what exactly you are trying to do - initializing, constructing, anything else. – anatolyg Nov 13 '17 at 16:53
  • 2
    Your `Copyable operator =` invokes undefined behavior. You are not returning a value when your function is supposed to return a `Copyable&`. – PaulMcKenzie Nov 13 '17 at 17:09

3 Answers3

2

You should accept CopyAble by reference at CopyAbleComposer(CopyAble m1), otherwise a copy constructor will be called to construct an argument. You should also mark it as explicit to avoid accidental invocations:

explicit CopyAbleComposer(const CopyAble & m1)
user7860670
  • 35,849
  • 4
  • 58
  • 84
  • @FrançoisAndrieux For this particular example there won't be much profit from move semantics though because only a single `int` is copied. – user7860670 Nov 13 '17 at 16:46
  • 2
    So, would you say that attempting to pass an object by value to a constructor would be incorrect / non-idiomatic as per C++? It still seems like a common trap people will fall into.. – SR Bhaskar Nov 13 '17 at 16:46
  • 2
    @programmerravi Passing objects by value typically should be avoided unless one wants to get a fresh new copy as an argument. It's fine for primitive types though. – user7860670 Nov 13 '17 at 16:48
2

Pass-by-value and the associated copying is a pretty widely known property of C++. Actually, in the past C++ was criticized for this gratuitious copying, which happened silently, was hard to avoid and could lead to decreased performance. This is humorously mentioned e.g. here:

You accidentally create a dozen instances of yourself and shoot them all in the foot. Providing emergency medical assistance is impossible since you can't tell which are bitwise copies and which are just pointing at others and saying, "That's me, over there."

C++98

When any function/method is declared to receive an argument by value, this sort of copying happens. It doesn't matter if it's a constructor, a "stand-alone" function or a method. To avoid this, use a const reference:

CopyAbleComposer(const CopyAble& m1) : memObj(m1)
{
    ...
}

Note: even if you rearrange your code as below, one copy always remains. This has been a major deficiency in C++ for a long time.

CopyAbleComposer cac(CopyAble(10)); // initializing mem1 by a temporary object

C++11

C++11 introduced move semantics, which replaces the additional copy by a "move" operation, which is supposed to be more efficient than copy: in the common case where an object allocates memory dynamically, "move" only reassigns some pointers, while "copy" allocates and deallocates memory.

To benefit from optimization offered by move semantics, you should undo the "optimization" you maybe did for C++98, and pass arguments by value. In addition, when initializing the mem1 member, you should invoke the move constructor:

    CopyAbleComposer(CopyAble m1) : memObj(std::move(m1)) {
        cout << "Composing the composer" << endl;
    }

Finally, you should implement the move constructor:

CopyAble(CopyAble&& obj) {
    cout << "Inside the CopyAble move constructor" << endl;
    this->mem1 = obj.mem1;
}

Then you should see that the "copy" message doesn't appear, and is replaced by the "move" message.

See this question for more details.

Note: In all these examples, the CopyAble objects are assumed to be much more complex, with copy and move constructors doing non-trivial work (typically, resource management). In modern C++, resource management is considered a separate concern, in the context of separation of concerns. That is, any class that needs a non-default copy or move constructor, should be as small as possible. This is also called the Rule of Zero.

anatolyg
  • 26,506
  • 9
  • 60
  • 134
0

Several issues:

  1. return *this; should not be in the copy constructor. It belongs in the assignement operator.
  2. CopyAbleComposer(CopyAble m1) is where a needless copy of ca is made. It should instead be CopyAbleComposer(const CopyAble& m1).
John Ko
  • 19
  • 2