Automatically implement operator+ using CRTP

Question

I have multiple classes that should support addition. For each of them, the operator += is implemented as T& operator+=(const T& rhs). I also want to add the operator +, but this implementation will use += and will therefore be similiar in all classes. My idea was to implement a template class using the Curiously Recurring Template Pattern that provides an implementation. The result is code like this:

template<class T>
class Sumable {
private:
    Sumable() = default;

public:
    Sumable(const Sumable<T>&) = delete;
    Sumable(Sumable<T>&&) = delete;

    T operator+(const T&) const&;
    T operator+(const T&) && ;
    T operator+(T&&) const&;
    T operator+(T&&) && ;

    friend T;
};

template<class T>
inline T Sumable<T>::operator+(const T & rhs) const&
{
    T result(*static_cast<const T*>(this));
    return std::move(result += rhs);
}

template<class T>
inline T Sumable<T>::operator+(const T & rhs) &&
{
    return std::move<T&>(*static_cast<T*>(this) += rhs);
}

template<class T>
inline T Sumable<T>::operator+(T && rhs) const&
{
    return std::move(rhs += *this);
}

template<class T>
inline T Sumable<T>::operator+(T && rhs) &&
{
    return std::move(rhs += *this);
}

Classes that support addition would extend from Sumable<T> like for example class Vector : public Sumable<Vector>

The operator is overloaded four times to prevent the creation of unnecessary temporary objects. The idea is described here https://stackoverflow.com/questions/6006527/overloading-on-r-value-references-and-code-duplication, but I return T by value to prevent the problems described in this question. I also follow this idea https://stackoverflow.com/questions/11224838/prevent-user-from-deriving-from-incorrect-crtp-base/11241079 to prevent incorrect use of the class.

In a simple test, this worked like expected. Are there any edge cases in which this design can cause problems? And is it a good idea to use this design in production code?

Edit The code I used to test this, compiles under MSVS 2017:

#include "stdafx.h"
#include <iostream>
#include "Sumable.h"

using namespace std;

struct Test : public Sumable<Test>{
    int data;

    Test(int data) : data(data) {};
    Test(const Test& that) : data(that.data) 
    {
        cout << "copy" << endl;
    };
    Test(Test&& that) : data(that.data) 
    {
        cout << "move" << endl;
    };
    Test& operator+=(const Test& that) {
        this->data += that.data;
        return *this;
    }
};
int main()
{
    Test a{ 1 };
    Test b{ 2 };
    Test c{ 3 };
    cout << (a + b + c + a + b + c).data << endl;
    return 0;
}

Answer 1

Missing header

I had to #include <utility> to get a definition for std::move().

Don't delete your destructors

You didn't show us your tests, but I struggled to get my obvious code to compile. Part of the reason was the deleted constructors; I changed the default constructor to protected and made the copy/move constructors to protected and = default, and then I was able to inherit without having to explicitly write copy/move construct/assign in my subclass:

struct Test : public Sumable<Test>
{
    int i;
    Test(int i) : i(i) {}
    Test& operator+=(const Test& t) { i += t.i; return *this; }
};

There's no need to move() when returning

I know the question is tagged C++11, but even there, any benefit is questionable (and particularly unlikely to exist when the function is inlined). When you move the code to a more recent standard, it's actively harmful.

Pass by value instead of copying from a reference

If you can insist on the usual x + y == y + x (which seems to be assumed already, given the third and fourth methods, which compute rhs += *this), then you can pass rhs by value (since we need to copy one of the arguments), which will be move-constructed as and when appropriate:

template<class T>
inline T Sumable<T>::operator+(T rhs) const
{
    return rhs += *static_cast<const T*>(this);
}

That's now one method instead of four.

N.B. I had to introduce the static_cast there, as your code wouldn't compile with GCC 8.0 without it.

Consider free functions

Inheritance isn't necessarily the best solution to everything in C++. You may be better writing free functions instead (in the same namespace as your classes, or in a specific includable namespace, along the lines of std::rel_ops).

Here's a start, including a basic test:

#include <type_traits>

namespace arithmetic_ops
{
    template<typename T, typename U>
    auto operator+(T t, U&& u)
        -> typename std::remove_reference<decltype(t+=u)>::type
    {
        return t += u;
    }

    template<typename T, typename U>
    auto operator+(T&& t, U u)
        -> typename std::enable_if<!std::is_assignable<T,U>::value,
                                   decltype(operator+(u,t))>::type
    {
        return u + t;
    }
}

namespace test {
    struct Test
    {
        int i;
        Test(int i) : i(i) {}
        Test& operator+=(const Test& t) { i += t.i; return *this; }
    };
}

int main()
{
    using namespace arithmetic_ops;

    test::Test a(4);
    test::Test b(-4);
    return (0 + a + b + 0).i;
}

I've used forwarding references above for the non-copied argument - that allows us to have only one method for the class-first case (the second version being an override for cases such as 0 + a where the first argument must be converted).

The enable_if avoids ambiguity when t + u and u + t are both possible.

The ::type and ::value members get shortened from C++14, with _t and _v suffixes to the names (e.g. std::remove_reference_t<decltype(t+=u)> and std::enable_if_t<!std::is_assignable_v<T,U>, decltype(operator+(u,t))>.

a and/or b can be made const in this test without breaking it.