A dangerous practice?

Question

Is this dangerous by any means? I have no knowledge of other way of doing it but it seems very suspicious.

class cA
{
public:
    cA(){}
    ~cA(){}
    int a;
//say class have tons of members..
};


int _tmain( int argc, _TCHAR* argv[] )
{
    cA obj;
    cA *ptr = new cA;
    *ptr = obj;
    //ofc there is a 'delete ptr;' after
}

If I remember right on C++ this means that an object of cA will be created and ptr will point to it, I have to do this to insert on long life containers ( vector<cA*> ).

Is copying the contents of obj from the stack to the heap that way valid?

edit possible solution?

class cA
{
public:
    cA(){}
    ~cA(){}
    int a;
    void Copy( cA & ref )
    {
        a = ref.a;
    }
};


int _tmain( int argc, _TCHAR* argv[] )
{
    cA obj;
    cA *ptr = new cA;
    ptr->Copy( obj );

Depends on what other members there are. If the class owns resources, yes. Document yourself on the rule of three. — Luchian Grigore, Jun 04 '13 at 01:30
Good point on the resources, any good ideas of how this could be safely done? I thought of creating a `Copy( cA& a)` method and manually copying all members. — Vinícius, Jun 04 '13 at 01:36
@ViniyoShouta: just use the standard copy constructor (either implicitly generated, or user-defined). As pointed out by Luchian you need to understand what is the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) (or Rule of Five in C++11). — syam, Jun 04 '13 at 01:40
*I thought of creating a `Copy( cA& a )` ...* - you *really* need to read a good [book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) on C++ — Praetorian, Jun 04 '13 at 01:54

score 2 · Answer 1 · answered Jun 04 '13 at 01:37

2

It's not a dangerous practice, however if you are the class author for cA, override operator= so that it does the proper copy.

Also along those same lines, implement a copy constructor.

answered Jun 04 '13 at 01:37

Alan

45,915
17
113
134

hookenz · Accepted Answer · 2013-06-26T03:57:56.507

2

Instead of this form:

cA obj;
cA *ptr = new cA;
ptr->Copy( obj );

Why not this form?

cA obj;
cA *ptr = new cA(ob);

Your copy constructor will look something like the following. There should also be an assignment operator.

class cA
{
public:
    cA(){}
    CA(const cA& ref)
    {
        a = ref.a;
    }
    ~cA(){}

    cA& operator=(const cA& p) {
        if (this != &p) {  // make sure not same object
            a = p.a;
        }
        return *this;    // Return ref for multiple assignment
    }

    int a;
};

And no what you were doing before was not dangerous, but perhaps just a little bit harder to understand.

edited Jun 26 '13 at 03:57

answered Jun 04 '13 at 02:00

hookenz

36,432
45
177
286

No its not harder at all, its much better, thanks a lot for this answer! – Vinícius Jun 04 '13 at 05:19
There's absolutely no point in writing copy constructor and copy assignment operator if the copy semantics is already properly implemented by the class data members. The OP did not post the actual content of the class, but what was posted in the example required no additional definitions. Moreover, there's nothing wrong with using the original assignment-based approach. – AnT stands with Russia Jun 26 '13 at 04:05
Of course, you're right. I was just being complete. Then if someone sees this and needs to do deep copies they can see where they would implement those. – hookenz Nov 14 '13 at 04:07

Preet Kukreti · Answer 3 · 2013-06-04T02:15:31.047

It's fine. But it depends.

You dont need to implement a copy constructor or assignment operator if your class is a plain old data type (e.g. if its all simple, non-pointer or non-resource-managing members). Say for example, if cA just holds a bunch of floats, ints and bool that you need to simply copy across the value of each member, then the automatic compiler-generated copy constructor will do fine.

If, however, it holds a pointer to some internal resource, then you need to implement a copy constructor and assignment operator that properly handles copying of the underlying resources (a "deep" copy).

You only need a virtual destructor if you are planning to inherit cA. Otherwise, you are just adding vtable pointer bloat to each instance of the class for no reason. If you dont intend to subclass, and are using C++11, you can set your class as final, and this means you dont have to worry about the virtual destructor issue if someone tries to subclass cA anyway (it will give them a compile error).

Furthermore, it can help to get into the practice of automatically declaring copy constructors and assignment operators as private when creating a new class. Then you get a compiler error when you try to do a copy assignment, and at that point you can just remove the declarations to allow the compiler to generate automatic versions. This way you have a bit more control and feedback on how your class is being used, and can avoid some surprises later on.

A dangerous practice?

edit possible solution?

3 Answers3