10
\$\begingroup\$

I wanted a simple thing: being able to override a base class and just add one single getter. The bad thing is the base class does override some operators and will return the wrong type. I, therefore, came across the curiously recurring template pattern which helped me to write the following.

However, I am still not very satisfied. I wrote a very similar question here which helped me to learn about CRTP.

#include <algorithm>
#include <array>
#include <iostream>

using namespace std;

template <typename C, typename T, int N>
class PointCRTP
{
   protected:
    array<T, N> elements;

   public:
    PointCRTP() = default;
    PointCRTP(array<T, N> el) : elements(el) {}

    C operator+(C& other)
    {
        C c;
        transform(elements.begin(), elements.end(), other.elements.begin(),
                  c.elements.begin(), plus<int>());
        return c;
    }

    T operator[](int k) { return elements[k]; }

    friend std::ostream& operator<<(std::ostream& os, C const& x)
    {
        os << "P(";
        for (auto k : x.elements) os << k << ", ";
        return os << "\b\b" << ')';
    }
};

template <typename T, int N>
struct Point : public PointCRTP<Point<T, N>, T, N> {
    using PointCRTP<Point<T, N>, T, N>::PointCRTP;
};

template <typename T>
struct Point2D : public PointCRTP<Point2D<T>, T, 2> {
    Point2D() = default;
    Point2D(T x, T y) : PointCRTP<Point2D<T>, T, 2>({x, y}) {}

    T getX() { return PointCRTP<Point2D<T>, T, 2>::elements[0]; }
    T getY() { return PointCRTP<Point2D<T>, T, 2>::elements[1]; }
};

int main()
{
    Point<int, 3> p({1, 2, 3});
    Point<int, 3> q({3, 4, 2});
    Point<int, 3> c = p + q;
    std::cout << c << " = " << p << " + " << q << endl;

    Point2D<int> r(1, 2);
    Point2D<int> s(3, 4);
    Point2D<int> t = r + s;
    std::cout << t << " = " << r << " + " << s << endl;
}

Is there a way to make this example simpler (shorter)?

\$\endgroup\$
8
\$\begingroup\$

Avoid special cases

A lot of problems come from the fact that you have slightly different ways to construct a 2D point and to get its elements than you have for an arbitrary dimensional point. You should avoid that first, this will make your life much easier.

Here is a class Point that supports getX() and getY() with compile-time checking that the size of elements is large enough, and a constructor that takes a variable number of arguments, so you don't need to use braces when constructing a Point:

template <typename T, int N>
class Point
{
   std::array<T, N> elements;

public:
    template<typename... E>
    Point(E&&...e): elements({std::forward<E>(e)...}) {}

    Point operator+(const Point& other)
    {
        Point c;
        std::transform(elements.begin(), elements.end(), other.elements.begin(),
                  c.elements.begin(), plus<T>());
        return c;
    }

    const T& operator[](size_t k) const
    {
        return elements[k];
    }

    friend std::ostream& operator<<(std::ostream& os, Point const& x)
    {
        os << "P(";
        for (auto& k : x.elements) os << k << ", ";
        return os << "\b\b" << ')';
    }

    template<std::enable_if_t<(N > 0), int> = 0>
    const T& getX() const {
            return elements[0];
    }

    template<std::enable_if_t<(N > 1), int> = 0>
    const T& getY() const {
            return elements[1];
    }
};

There are various ways to enable/disable member functions at compile time, the above uses std::enable_if_t which completely disables a function. You can also use a compile-time assert, like so:

T getY() const {
    static_assert(N > 1, "this point doesn't have a y coordinate");
    return elements[1];
}

Although as pointed out in the comments, this has the drawback that you cannot check at compile time if a given Point type supports getX(), getY().

To make a Point2D convenience class, use using:

template<typename T>
using Point2D = Point<T, 2>;
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Alternative to the static_assert()s: template <int X= 0> std::enable_if_t<(X == 0 && X < N), T> getX() ... dito for getY(). \$\endgroup\$ – Deduplicator Nov 5 '20 at 0:04
  • 1
    \$\begingroup\$ Personally I think the static_assert() makes the code look nicer, plus you get to define a custom error message. Although disabling the function entirely is also not bad. \$\endgroup\$ – G. Sliepen Nov 5 '20 at 0:11
  • 2
    \$\begingroup\$ Using SFINAE instead of static_assert has the advantage of being compile-time checkable using traits (e.g. supports_getX<T>). \$\endgroup\$ – Angew is no longer proud of SO Nov 5 '20 at 8:21
  • 1
    \$\begingroup\$ @AngewisnolongerproudofSO You convinced me :) \$\endgroup\$ – G. Sliepen Nov 5 '20 at 10:08
8
\$\begingroup\$

Assignment using [] operator

If you have a look at your overload of []

    T operator[](size_t k) const
    {
        return elements[k];
    }

You are creating a copy. What you should do is returning a reference - & otherwise, you can't do something like

Point< int, 3 > p({1,2,3});
p[0] = 5; // Erorr: Expression must be a modifiable value

Simply solve it by returning a reference

    T& operator[](size_t k)
    {
        return elements[k];
    }

As pointed at by @AI0867 in the comments, since we're returning a modifiable value we need to remove const.


Avoid copies

From your overload of the << operator

for (auto k : x.elements) os << k << ", ";

auto k performs a deep copy, which would be fine if you had a primitive type like int, double. But anything large, like std::string will make things slow.

for(auto& k : x.elements)

The same goes for

Point2D(T x, T y) : PointCRTP<Point2D<T>, T, 2>({ x, y }) {}
\$\endgroup\$
2
  • \$\begingroup\$ You've just made operator[] return a modifiable reference while keeping it const. You probably want to remove the const, or make both a const and a non-const version, one of which returns a const-ref and the other a modifable reference. \$\endgroup\$ – AI0867 Nov 5 '20 at 13:11
  • 1
    \$\begingroup\$ @AI0867 Thank you for pointing that out!, it should be alright now \$\endgroup\$ – Parekh Nov 5 '20 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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