copy constructor for a class with an abstract member

[hebrerillo]

Hi everyone!

This question is related to how implement a copy constructor for a class that has a pointer to an abstract type. Here is an example:

class MyAbstract
{
public:    
    virtual void abstractMethod() const = 0;

};

class UseAbstract
{
public:
    UseAbstract (MyAbstract *abstract, int b)
    {
        abstract_ = abstract;
        b_ = b;
    }
    
    UseAbstract (const UseAbstract &other)
    {
        b_ = other.b_;
        //abstract_ = new ??
    }
    
private:
    MyAbstract *abstract_;
    int b_;
};

How can I implement the copy constructor for UseAbstract? I can't create an object for 'MyAbstract'.

Thanks a lot!

[Repeater]

Edit: Gargh... code tidy needed...

More edit: And then JLB gave a link to far neater clone code than I was writing :)

[kbw]

It depends on what abstract_ is. What does the destructor for that class look like?

[JLBorges]

Use the clone idiom: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Virtual_Constructor

That example is a bit dated; if it is an owning pointer, ideally use std::unique_ptr<>
in conjunction with std::make_unique<>

[hebrerillo]

Hi everyone,

I am sorry I did not specify who owns and free the memory. Here is an example code

#include <iostream>

class MyAbstract
{
public:

    MyAbstract()
    {
    }

    MyAbstract& operator=(const MyAbstract & other)
    {
        if (this != &other)
        {
            copy(other); //invoke the subclass copy method
        }
        return *this;
    }

    virtual void copy(const MyAbstract & other) = 0;
    virtual void abstractMethod() const = 0;

};

class Implement : public MyAbstract
{
public:
    int a_;

    Implement(int a)
    {
        a_ = a;
    }

    Implement(const Implement &other)
    {
        copy(other);
    }
    
    /*!
     * This method is called from the parent operator=
     */
    void copy(const MyAbstract & other)
    {
        const Implement *aux = (const Implement *) & other;
        a_ = aux->a_;
    }

    void abstractMethod() const
    {
        std::cout << "value of a_ = " << a_ << std::endl;
    }
};

class UseAbstract
{
public:

    UseAbstract(MyAbstract *abstract)
    {
        abstract_ = abstract;
    }

    UseAbstract& operator=(const UseAbstract &other)
    {
        if (this != &other)
        {
            *abstract_ = *(other.abstract_);
        }
        return *this;
    }

    void useAbstractMethod()
    {
        abstract_->abstractMethod();
    }

private:

    //private because abstract_ is uninitialized
    UseAbstract(const UseAbstract &other)
    {
        //this causes a crash
        //*abstract_ = *(other.abstract_);
    }
    
    MyAbstract *abstract_;

};

int main()
{
    Implement * abstract1 = new Implement(1);
    Implement * abstract2 = new Implement(2);

    UseAbstract useabstract1(abstract1);
    UseAbstract useabstract2(abstract2);

    useabstract2.useAbstractMethod();
    useabstract2 = useabstract1;
    useabstract2.useAbstractMethod();

    delete abstract1;
    delete abstract2;
    return 1;
}

The management of the memory is done by the client code, in this case 'main'.

What are your thoughts about this code?

Be aware that the copy constructor is private in UseAbstract. Because I do not want the client code to do something like this

UseAbstract useabstract3 = useabstract2; //crash 

JLBorges the clone idiom you talk about is very interesting, but I do not want to use it because I want the memory be managed by the client of the class ('main' here).

Thanks a lot!

[kbw]

Ok, so UseAbstract::abstract_ is just a pointer to something external. It's not managing an allocated object or block of memory, so the copy constructor is:

UseAbstract::UseAbstract(const UseAbstract &other) :
    b(other.b),
    abstract_(other.abstract)
{
}

And assignment is:

UseAbstract& UseAbstract::operator=(const UseAbstract &other)
{
    if (this != &other) {
        b = other.b;
        abstract_ = other.abstract_;
    }

    return *this;
}

[Repeater]

The management of the memory is done by the client code, in this case 'main'.

What are your thoughts about this code?

Thoughts; manual memory management is a bad thing. I think it should be heavily, heavily avoided. Create objects on the stack where you can, manage them with smart pointers where you must, and use raw memory management carefully wrapped in thin manager classes where you have to work with legacy (i.e. manual memory management relying on meatspace enforcement). Writing brand new legacy code, such as this, is bad.

[JLBorges]

    void copy(const MyAbstract & other)
    {
        const Implement *aux = (const Implement *) & other;
        a_ = aux->a_;
    }

This will result in undefined behaviour if the dynamic type of the object referenced by other
is not Implement or a class derived from Implement

The code is also brittle; it would break if the base class is modified some time later to have a non-trivial copy.

It is usually quite messy to implement full value semantics on object oriented types;
but if we must, something along these lines would be required:

#include <iostream>
#include <string>

struct abstract
{
    abstract() = default ;
    abstract( const abstract& ) = default ;
    abstract( abstract&& ) = default ;
    virtual ~abstract() = default ;

    abstract& operator= ( const abstract& that ) { return copy(that) ; }
    abstract& operator= ( abstract&& that ) noexcept { return move( std::move(that) ) ; }

    virtual std::string to_string() const = 0 ;

    protected:
        virtual abstract& copy( const abstract& ) { return *this ; } // nothing to be copied
        virtual abstract& move( abstract&& ) noexcept { return *this ; } // nothing to be moved
};

struct impl : abstract
{
    explicit impl( int a = 0, std::string b = {} ) : a(a), b( std::move(b) ) {}

    virtual std::string to_string() const override
    { return "impl{" + std::to_string(a) + ',' + b + '}' ; }

    private:
        int a ;
        std::string b ;

    protected:
        virtual impl& copy( const abstract& that ) override
        {
            std::cout << "impl::copy\n" ;

            abstract::copy(that) ; // copy the base class part (if any)

            // if the dynamic type of that is impl or impl-derived, copy the derived class part
            auto p = dynamic_cast<const impl*>( std::addressof(that) ) ;
            if(p) { a = p->a ; b = p->b ; }
            else { std::cout << "\tincompatible type: ignored\n" ; }

            return *this ;
        }

        virtual impl& move( abstract&& that ) noexcept override
        {
            std::cout << "impl::move\n" ;

            // if the dynamic type of that is impl or impl-derived, move the derived class part
            auto p = dynamic_cast<const impl*>( std::addressof(that) ) ;
            if(p) { a = p->a ; b = std::move(p->b) ; }
            else { std::cout << "\tincompatible type: ignored\n" ; }

            abstract::move( std::move(that) ) ; // move the base class part (if any)

            return *this ;
        }
};

http://coliru.stacked-crooked.com/a/355b7a5a52ff0dff