views:

120

answers:

6

My objective is to do a deep copy of a class, but a virtual class is causing trouble.

#include<iostream>
using namespace std;

class Vir//pure virtual class
{
    public:
    virtual void hi()=0;
};

class Handler:public Vir
{
    public:
    int i;
    Handler() {}
    Handler(int val):i(val) {}
    void hi() {cout<<"Value of i="<<i<<endl;}
    int getI() const {return i;}
    void setI(int j) {i=j;}
};

class ControlPanel
{
    public:
    Vir *v;
    ControlPanel(const ControlPanel& c)//copy constructor
    {
        v=new Handler;
        v->setI(c.getI());
    }
    int getI()const {return v->getI();}

    void initialize() {v=new Handler(10);}
    void hi() {v->hi();}
    ControlPanel() {}
    ~ControlPanel() {delete v;}
};

int main()
{
    ControlPanel cc;
    cc.initialize();
    cc.hi();
    ControlPanel bb(cc);//copying cc into bb
}

The compilation error message:

test.cpp: In copy constructor ‘ControlPanel::ControlPanel(const ControlPanel&)’:
test.cpp:28: error: ‘class Vir’ has no member named ‘setI’
test.cpp: In member function ‘int ControlPanel::getI() const’:
test.cpp:30: error: ‘class Vir’ has no member named ‘getI’

I plan to have plenty more Handler classes (like Handler1, Handler2 etc) which inherit from Vir and will have their own unique members (like float a; or double b; etc). So it doesn't make sense for me to keep all the getter & setter functions of all Handler classes in the Vir class. I want to keep my getter and setter methods in the Handler classes because the members are unique to the Handler classes. The compiler is not allowing me to do so. Help?

A: 

Why would the compiler allow you? Those methods are not on that interface.

You could use the Factory Pattern to create your Vir, to avoid having to add all of the constructors to Vir's interface. You should also consider using RAII to avoid initialize() style functions.

luke
Thanks Luke. Will consider re-designing.
Nav
Tried the factory pattern and the prototype pattern. Both don't solve the problem. I'll still have to upcast the returned object, won't I? That's where the problem is. As for RAII, I'll definitely use it.
Nav
What's the purpose of holding a pointer to the abstract class `Vir` if you always assign it to a `Handler`? It'd make more sense if you were passing in a pre-constructed `Vir` (via one of the patterns you described), or just had a regular `Handler` instance in `ControlPanel`. Is the need for that simplified away by your example?
luke
+1  A: 

Change Vir *v to Handler *v; and see whether your code compiles or not.

Your class Vir doesn't declare/define setI() and getI() member functions.

Or define Vir as

class Vir//pure virtual class
{
    public:
    virtual void hi()=0;
    virtual int getI()const =0;
    virtual void setI(int)=0;
};
Prasoon Saurav
The point of having Vir *v is so that I can later create some Handler1 and Handler2 classes and dynamically assign them to v, like so: v=new Handler2; That's the whole point of using virtual!
Nav
@user453673 : Add to your class `Vir` pure virtual declarations of `setI()` and `getI()`. See the edits.
Prasoon Saurav
Please read my question completely. My whole purpose of posting was to avoid what you've suggested.
Nav
@user453673 : Then I dont think there is any way. Sorry!
Prasoon Saurav
A: 

You have to define getI and setI as (pure) virtual in Vir to make them accessible via subclasses. No way around this.

Steve Townsend
"No way around this" - darn!
Nav
Nothing is impossible: The best solution to the question I posted is this: ControlPanel(const ControlPanel} and the Handler class copy constructor is Handler(const Handler}
Nav
@Nav - fyi you can post this info as your accepted answer to your own question, if that's the way you went.
Steve Townsend
Done. Thanks :) Some votes would be appreciated :)
Nav
+2  A: 

Add a duplicate() function to your abstract class, which (in each derived class) creates a new instance with the right values and returns it. Alternatively, consider a copyFrom(Abs other) function which checks to ensure that you're copying from the correct type and if so, copies the fields out.

In general, if your ControlPanel class has a reference to an Abs object, it shouldn't be trying to do its duplication by inspecting the concrete Handler object, it should be passing the duplication off to a virtual function on that object.

Andrew Aylett
Thanks, I'm just adding a link for anyone else who sees your post and wants to know what context Abs was used in. Link: http://stackoverflow.com/questions/3829220/is-there-no-way-to-upcast-into-an-abstract-class-and-not-modify-it-each-time-a-cl
Nav
I didn't get what you meant by 'passing the duplication off to a virtual function on that object'. Did you mean that the duplicate() function in Vir should have a corresponding duplicate() function in Handler, and Handler's function returns an object of handler? Type mismatches is what worries me here.
Nav
The function would have a return type of `Abs` (so you can declare it in the superclass) but you're okay returning a `Handler` object from `Handler`'s implementation because `Handler` inherits from `Abs`.
Andrew Aylett
+1: A virtual clone/duplicate method is the nicest way to do it.
Troubadour
Thanks Andrew. Your answer was the best, but I didn't understand how to use it until Troubadour explained it. Thanks guys!
Nav
A: 

As Steve suggested, I'm answering my own question coz a friend gave me a solution. Hope this would be of help to anyone who has the question of how to do a Deep copy in C++ where a virtual class may be a roadblock. Hope someone finds this useful.

#include<iostream>
using namespace std;

class Vir//pure virtual class
{
    public:
    virtual void hi()=0;
    virtual int getI() {std::cout << "Inside Base class" << std::endl;}
    virtual void setI(int i) {cout<<"In base"<<endl;}
    virtual int getX() {}
    virtual void setX(int x) {}
};
class Model
{
    public:
    int x;
    Model(const Model& mm) {x=mm.x;}
    Model():x(555) {cout<<"Model constructor called"<<endl;}
    int getX() {return x;}
    void setX(int xx) {x=xx;}
};

class Handler:public Vir
{
    public:
    int i;
    Model *m;

    Handler() {m=new Model;cout<<"Handler constructor called"<<endl;}
    Handler(const Handler& h)
    {
    std::cout << "Inside Handler @lineNumber:" << __LINE__ << std::endl;
    i=h.i;
    m=new Model(*h.m);
    }
    Handler(int val):i(val) {}
    ~Handler() {delete m;}
    void hi() {cout<<"Value of i="<<i<<endl;}
    int getI() {return i;}
    void setI(int j) {i=j;}
    int getX() {return m->getX();}
    void setX(int xx) {m->setX(xx);}
};

class ControlPanel
{
    public:
    int abc;
    Vir *v;
    ControlPanel(const ControlPanel& c)//copy constructor
    {
    std::cout << "Inside ControlPanel @lineNumber:" << __LINE__ << std::endl;
        v=new Handler((Handler&)*(c.v));
    }
    void initialize() {v=new Handler();v->setI(10);abc=222;}
    void hi() {v->hi();}
    ControlPanel() {}
    ~ControlPanel() {delete v;}
};

int main()
{
    ControlPanel cc;
    cc.initialize();
    cc.hi();    
    cout << "(cc.v)->i::" << (cc.v)->getI() << endl;
    cout<<"x value cc="<<(cc.v)->getX()<<endl;
    ControlPanel bb(cc);//copying cc into bb
    cout << "(bb.v)->i::" << (bb.v)->getI() << endl;
    cout<<"x value bb="<<(bb.v)->getX()<<endl;
    (cc.v)->setI(999);
    (cc.v)->setX(888888888);
    cout << "(cc.v)->i::" << (cc.v)->getI() << endl;
    cout << "(bb.v)->i::" << (bb.v)->getI() << endl;
    cout<<"x value cc="<<(cc.v)->getX()<<endl;
    cout<<"x value bb="<<(bb.v)->getX()<<endl;
}//main
/*
Output:
Model constructor called
Handler constructor called
Value of i=10
(cc.v)->i::10
x value cc=555
Inside ControlPanel @lineNumber:52
Inside Handler @lineNumber:32
(bb.v)->i::10
x value bb=555
(cc.v)->i::999
(bb.v)->i::10
x value cc=888888888
x value bb=555  */
Nav
luke
The same holds true for ControlPanel, and it's Vir member. It will always point to an instantiation of of a Handler. I don't see the need for it to be a pointer, or based on an abstract class.
luke
This is not the best way to do it. [@Andrew Aylett's answer](http://stackoverflow.com/questions/3813964/cant-copy-construction-be-done-without-creating-an-explicit-function-in-the-pure/3829394#3829394) is superior and should be the accepted answer IMO. If you want details then have a look at [my answer](http://stackoverflow.com/questions/3813964/cant-copy-construction-be-done-without-creating-an-explicit-function-in-the-pure/3839259#3839259).
Troubadour
@luke: I initialized the Model and Vir as pointers coz I thought that it's always better to have classes with a long life, on the heap rather than the stack. This program is a highly scaled down version of what I'm actually making, and Model and Vir will have a very long lifetime. The need for the abstract class is coz I have many more handlers Handler1, Handler2 etc. So with the abstract class, I can just call v.hi() instead of if (handler) h.hi(); else if (handler1) h1.hi(); ...and so on
Nav
Question: Agreed that the clone() technique is better, but in the code above, won't "v=new Handler((Handler" work faster than clone coz clone works on the premise of late binding, and the compiler would have to generate extra code at runtime to find the correct clone function. But if I use the cast like how I've done above, the compiler won't have to do any late binding, and the function would work faster than clone. Am I right?
Nav
+2  A: 

Maybe I am missing something but would you not be better with a virtual clone method on Vir? This means you can avoid the nasty cast in the ControlPanel copy constructor outlined in your own answer. This is the same as @Andrew Aylett suggests in his answer with duplicate being used instead of clone.

Something like

class Vir
{
    public:
    virtual Vir* clone() const = 0;
    ...
};

which is implemented in Handler to be

Handler* Handler::clone() const
{
    return new Handler( *this );
}

Note the use of the covariant return type i.e. Handler::clone is allowed to return a Handler* rather than just a Vir* and still be a valid override of Vir::clone.

This makes the ControlPanel copy constructor simply

ControlPanel( const ControlPanel& c )
    : v( c.v->clone() )
{
}
Troubadour
OH...MY...GOD!!! It actually WORKED!!! :) Thanks a zillion Troubadour! The clone method was suggested earlier too, but I just didn't see how it could be used because Vir's clone would have Vir* as it's return type. Your explanation cleared up all the doubts. Absoulutely LOVE this solution! Thanks :) Accepted answer. Could everyone vote up this answer please?
Nav