C++ operator overloading in recursive expression classes

Question

I have a recursive class Expression which denotes boolean-like expressions, for instance:

(a & b) | (c & ~d)

Note that Expression takes care of both unary and binary expressions. Basically, Expression should follow the CFG similar to a boolean expression.

I have designed the class in this way:

class Expression {
public:
    Expression() = default;
    Expression(unique_ptr<Expression> lhs, unique_ptr<Expression> rhs,
        unique_ptr<IBinaryOperator> binop, unique_ptr<IUnaryOperator> unop);
    Expression operator^(Expression& that);
    Expression operator%(Expression& that);
    Expression operator|(Expression& that);
    Expression operator*(Expression& that);
    Expression operator+(Expression& that);
    Expression operator&(Expression& that);
    Expression operator>>(Expression& that);
    Expression operator!();
    Expression operator~();
    double Evaluate(double x);
    virtual ~Expression();
protected:
    unique_ptr<Expression> _lhs = nullptr;
    unique_ptr<Expression> _rhs = nullptr;
    unique_ptr<IBinaryOperator> _binop = nullptr;
    unique_ptr<IUnaryOperator> _unop = nullptr;
};

The implementation of the constructor and one each of the binary and unary operators are shown below:

Expression::Expression(unique_ptr<Expression> lhs, unique_ptr<Expression> rhs, unique_ptr<IBinaryOperator> binop, unique_ptr<IUnaryOperator> unop) :
        _lhs(move(lhs)), _rhs(move(rhs)), _binop(move(binop)), _unop(move(unop)) {
}
Expression Expression::operator+(Expression&& that) {
    return Expression(unique_ptr<Expression>(this), unique_ptr<Expression>(&that), unique_ptr<IBinaryOperator>(new SumCoNorm), nullptr);
}
Expression Expression::operator~() {
    return Expression(nullptr, unique_ptr<Expression>(this), nullptr, unique_ptr<IUnaryOperator>(new Intensify));
}

The class fails to compile with

error: use of deleted function 'Fuzzy::Expression::Expression(const Fuzzy::Expression&)'

in each of the overloaded operators (in the return statements). I get the feel that some function is internally trying to use the copy constructor of unique_ptr, which does not exist. Am I doing something wrong with moving pointers here and there? I am using C++11 with GCCv4.8.

Suggestions to changes in class interface in any way is welcome. I would prefer avoiding the use of raw pointers.

Note: Please do not suggest using a parser generator or the like, such as Boost.Spirit, YARD or YACC. The application requires me to implement this from scratch.

Answer 1

Conceptually,

return Expression(...);

creates a new Expression object, and then copies or moves it to the return value. In your case, you have no move constructor (there is no implicit move constructor since you have a user-declared destructor), and a deleted copy constructor, so this is not possible.

You can use

return {...};

to avoid the copy/move operation, or you can make sure you have a move constructor:

class Expression {
public:
    Expression() = default;
    Expression(Expression &&) = default;
...
};

Additional note, after comments from Ben Voigt who rightly points out that this makes it compile, but not actually work:

unique_ptr only works for objects allocated with new, unless you use a custom deleter. In your case, it just won't work, you'll need to re-think your logic.

I think you should keep unique_ptr only as an implementation detail, and not have outside callers worry about it. If you ensure your Expression is copyable and movable, you should have no problems dynamically allocating objects as needed from within Expression to store as _lhs and _rhs. Take std::vector<T> as an example, where you don't need new to use it, even though for obvious reasons, adding enough elements will at some point necessarily start requiring dynamic memory allocations.

Answer 2

Your problem is that you are mixing temporary objects which live in their own scope and objects owned by a unique_ptr:

Expression Expression::operator+(Expression&& that) {
    return Expression(unique_ptr<Expression>(this),
                      unique_ptr<Expression>(&that),
                      unique_ptr<IBinaryOperator>(new SumCoNorm),
                      nullptr);
}

You want to return a new Expression object as a temporary and you have the existing object (through *this) and you have that. You want to take ownership but you can't and hence the compiler tries to create a copy. The temporary would be destructed anyways and you can't prevent that, so you can't take ownership by putting a pointer to it into a unique_ptr.

What you would need is something like

// Note: free function taking *two* operands
unique_ptr<Expression> operator+(unique_ptr<Expression> lhs,
                                 unique_ptr<Expression> rhs) {
    return unique_ptr<Expression>(
      new Expression(std::move(lhs),
                     std::move(rhs),
                     unique_ptr<IBinaryOperator>(new SumCoNorm),
                     nullptr));
}

and handle the final result accordingly.

Answer 3

Step 1: Expression(Expression&&)=default.

Step 2: Clauses like unique_ptr<Expression>(this) become unique_ptr<Expression>(new Expression(std::move(*this))), and unique_ptr<Expression>(&that) becomes unique_ptr<Expression>(new Expression(std::move(that))).

Now, this has exceptions safety issues, so you'll want write:

template<typename T, typename... Args>
std::unique_ptr<T> make_unique(Args&&...args) {
  return {new T(std::forward<Args>(args)...)};
}

which both makes things safer, and lets you do this:

make_unique<Expression>(std::move(*this))
make_unique<Expression>(std::move(that))

which is simpler and safer.

Next, you need to distinguish between Expression& and Expression&& pretty much everywhere.

You might be better off learning about expression templates, and using that technique. This would let you have expressions that look like x + 5 or the like.