CODEWITHSUNDEEP
c++exceptionclone
Anna
Anna

Reputation: 2845

How to clone subclasses without duplicating code?

Class Plus inherits from class Expression. 

class Expression
{
 public:
  virtual Expression* clone() const = 0;
};
class Plus : public Expression
{
 public:
 Plus( Expression* lhs, Expression* rhs ) :Expression( lhs, rhs) {};
 Plus* clone() const;
};

I'm implementing a copy function. The current implementation will leak memory if one of the clones or the Plus fails.

Plus* Plus::clone() const  {
        return new Plus(tree_left->clone(), tree_right->clone());
}

and I think something like this would solve the problem:

Plus* Plus::clone() const  {
        Expression *tree_left_clone = nullptr;
        Expression *tree_right_clone = nullptr;
        Expression *plus_clone = nullptr;
        try {
             tree_left_clone = tree_left->clone();
             tree_right_clone = tree_right->clone()
             plus_clone = new Plus( tree_left_clone, tree_right_clone );
        } catch (const bad_alloc& e ) {
             delete tree_left_clone;
             delete tree_right_clone;
             delete plus_clone;
        }
        return plus_clone;
}

But there's a lot of similar operators including Minus, Times, Divided, Power etc. And I'd have to duplicate clone for all of them. Is there a way to put this code in Expression? The problem I'm having is with the line in clone containing new Plus.

Upvotes: 2

Views: 83

Answers (3)

Slava
Slava

Reputation: 44268

Use smart pointers instead of raw ones and your code will not leak:

class Expression;
typedef std::shared_ptr<Expression> ExpressionPtr;
class Expression
{
 public:
  virtual ExpressionPtr clone() const = 0;
};
class Plus : public Expression
{
 public:
 Plus( ExpressionPtr lhs, ExpressionPtr rhs ) :Expression( lhs, rhs) {};
 ExpressionPtr clone() const;
};

ExpressionPtr Plus::clone() const  {
    return std::make_shared<Plus>( tree_left->clone(), tree_right->clone() );
}

You can use boost::shared_ptr if c++11 is not available.

Upvotes: 1

Daniel Frey
Daniel Frey

Reputation: 56883

That's what smart pointers are for. You could change the constructor to receive a std::unique_ptr:

Plus( std::unique_ptr<Expression> lhs, std::unique_ptr<Expression> rhs )
  : Expression( lhs.release(), rhs.release() ) {}

and call it like this:

Plus* Plus::clone() const {
    std::unique_ptr<Expression> lhs( tree_left->clone() );
    std::unique_ptr<Expression> rhs( tree_right->clone() );
    return new Plus( std::move(lhs), std::move(rhs) );
}

But that should only be the beginning. You should also consider storing the pointers as std::unique_ptr and avoid plain pointers as much as possible.

Upvotes: 1

dnk
dnk

Reputation: 661

Try something like that:

    Plus* Plus::clone() const  {
         auto_ptr<Expression> tree_left_clone(tree_left->clone());
         auto_ptr<Expression> tree_right_clone(tree_right->clone());
         return new Plus(tree_left_clone.release(), tree_right_clone.release());
    }

Upvotes: 1

Related Questions