10
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ What you're trying to create is kind of common and has a name: Barton-Nackman Trick. I think the article might provide some insight and a Google with the name might give some code to look at. Best wishes. \$\endgroup\$
    – Emily L.
    Apr 2, 2018 at 10:41

1 Answer 1

4
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ There's no need to move() when returning: When I tested this code and logged calls to copy-/move-constructor, removing the std::move resulted in the creation of a temporary copy. \$\endgroup\$
    – Feanor
    Apr 2, 2018 at 15:07
  • 1
    \$\begingroup\$ That might be a Quality of Implementation issue with your compiler and optimisation level. If you find you need it on some platforms, I recommend you at least wrap it in a macro so that it's not inflicted on better systems. (i.e. #define RETURN_MAYBE_MOVE(x) return (x), with #define RETURN_MAYBE_MOVE(x) return std::move(x) where you must. \$\endgroup\$ Apr 2, 2018 at 15:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.