3
\$\begingroup\$

I am learning about templates in C++ so I decided to implement an N-dimensional vector. The code seems to work fine, but there are a few things that I am unsure about.

To stop GetW() being called on a 3-dimensional vector, I used std::enable_if. This works, but the error message given when it's misused isn't as helpful as I'd like: error: no type named ‘type’ in ‘struct std::enable_if<false, void>’. It would be better if I could make it into something friendlier like: error: attempted to get 4th component of a 3d vector instead.

I am also unsure about the best practices with templates, so any feedback would be appreciated. Thank you :)


#include <array>
#include <cassert>
#include <cmath>
#include <type_traits>

template<
    typename T,
    int num_components,
    typename = typename std::enable_if<std::is_arithmetic<T>::value, T>::type,
    typename = typename std::enable_if<(num_components >= 2)>::type
>
class Vector {
    using this_t = Vector<T, num_components>;

public:
    Vector() : components{0} {}
    Vector(std::array<T, num_components> components) : components(components) {}

    template<int idx>
    inline T Get() const {
        typename std::enable_if<(num_components >= idx)>::type();
        return this->components[idx];
    }
    inline T Get(int idx) const {
        assert(idx <= num_components);
        return this->components[idx];
    }
    inline T GetX() const {
        return this->components[0];
    }
    inline T GetY() const {
        return this->components[1];
    }
    inline T GetZ() const {
        typename std::enable_if<(num_components >= 3)>::type();
        return this->components[2];
    }
    inline T GetW() const {
        typename std::enable_if<(num_components >= 4)>::type();
        return this->components[3];
    }
    template<int idx>
    inline void Set(T value) {
        typename std::enable_if<(num_components >= idx)>::type();
        this->components[idx] = value;
    }
    inline void Set(int idx, T value) {
        assert(idx >= num_components);
        this->components[idx] = value;
    }
    inline void SetX(T value) {
        this->components[0] = value;
    }
    inline void SetY(T value) {
        this->components[1] = value;
    }
    inline void SetZ(T value) {
        typename std::enable_if<(num_components >= 3)>::type();
        this->components[2] = value;
    }
    inline void SetW(T value) {
        typename std::enable_if<(num_components >= 4)>::type();
        this->components[3] = value;
    }

    double LengthSquared() const {
        double ret = 0;
        for (int i = 0; i < num_components; i++) {
            const T value = this->components[i];
            ret += value * value;
        }
        return ret;
    }
    double Length() const {
        return std::sqrt(this->LengthSquared());
    }

    void Normalise() {
        const double length = this->Length();
        for (int i = 0; i < num_components; i++) {
            this->Set(i, this->Get(i) / length);
        }
    }
    this_t Normalised() const {
        const double length = this->Length();
        std::array<T, num_components> new_components;
        for (int i = 0; i < num_components; i++) {
            new_components[i] = this->Get(i) / length;
        }
        return this_t(std::move(new_components));
    }

    void Negate() {
        for (int i = 0; i < num_components; i++) {
            this->components[i] = -this->Get(i);
        }
    }
    this_t Negated() const {
        std::array<T, num_components> new_components;
        for (int i = 0; i < num_components; i++) {
            new_components[i] = -this->Get(i);
        }
        return this_t(std::move(new_components));
    }

    this_t operator+(const this_t &r) const {
        std::array<T, num_components> new_components;
        for (int i = 0; i < num_components; i++) {
            new_components[i] = this->Get(i) + r.Get(i);
        }
        return this_t(std::move(new_components));
    }
    this_t operator-(const this_t &r) const {
        std::array<T, num_components> new_components;
        for (int i = 0; i < num_components; i++) {
            new_components[i] = this->Get(i) - r.Get(i);
        }
        return this_t(std::move(new_components));
    }
    this_t operator*(const this_t &r) const {
        std::array<T, num_components> new_components;
        for (int i = 0; i < num_components; i++) {
            new_components[i] = this->Get(i) * r.Get(i);
        }
        return this_t(std::move(new_components));
    }
    this_t operator*(T s) const {
        std::array<T, num_components> new_components;
        for (int i = 0; i < num_components; i++) {
            new_components[i] = this->Get(i) * s;
        }
        return this_t(std::move(new_components));
    }
    bool operator=(const this_t &r) const {
        for (int i = 0; i < num_components; i++) {
            if (this->Get(i) != r.Get(i)) return false;
        }
        return true;
    }

    this_t Cross(const this_t &r) const {
        // pretend that cross product is only defined for a 3-dimensional vector
        typename std::enable_if<(num_components == 3)>::type();
        std::array<T, num_components> new_components;
        new_components[0] = this->GetY() * r.GetZ() - this->GetZ() * r.GetY();
        new_components[1] = this->GetZ() * r.GetX() - this->GetX() * r.GetZ();
        new_components[2] = this->GetX() * r.GetY() - this->GetY() * r.GetX();
        return this_t(std::move(new_components));
    }

    T Dot(const this_t &r) const {
        T ret = 0;
        for (int i = 0; i < num_components; i++) {
            ret += this->Get(i) * r.Get(i);
        }
        return ret;
    }
private:
    std::array<T, num_components> components;
};

using Vector2d = Vector<double, 2>;
using Vector2f = Vector<float, 2>;
using Vector2i = Vector<int, 2>;
using Vector3d = Vector<double, 3>;
using Vector3f = Vector<float, 3>;
using Vector3i = Vector<int, 3>;
using Vector4d = Vector<double, 4>;
using Vector4f = Vector<float, 4>;
using Vector4i = Vector<int, 4>;
\$\endgroup\$
  • 1
    \$\begingroup\$ It looks a lot like what the GLM library (glm.g-truc.net) does. It might be helpful to have a look at its implementation. \$\endgroup\$ – G. Sliepen Oct 4 '18 at 11:12
3
\$\begingroup\$

Comments:

I am learning about templates in C++ so I decided to implement an N-dimensional vector.

It's not an N-dimensional vector. It's a 1-D vector with N elements.

OK. Now I have read this for a while. I am starting to think "Mathematical Vector". Is that what you mean? But even that is not quite correct, as a "Mathematical vector" has a direction (which you can specify with 3 lengths); but you also need a point for it to go through (so you need another 3 points to define that). Still confused.

To stop GetW() being called on a 3-dimensional vector, I used std::enable_if. This works, but it produces a very nasty error message: error: no type named ‘type’ in ‘struct std::enable_if<false, void>’.

This is because it is a compile time check. It's supposed to be readable by developers. That's because it is supposed to stop developers from making mistakes (not users).

It would be better if I could make it into something friendlier like: error: attempted to get 4th component of a 3d vector instead.

This should be available with a C++ feature called Concepts. Unfortunately this is not scheduled until 2020 (if it is not cancelled again).

I am also unsure about the best practices with templates, so any feedback would be appreciated. Thank you :)

That's what we are here for.

Review:

My compiler generates a warning on this line:

    Vector() : components{0} {}

components is expecting a list here, not a single element. It can be a list with a single element.

Pass by reference:

This creates a copy of the parameters. Then the parameters are copied into the member variable. Its possible the compiler may optimize this, but don't count on it.

    Vector(std::array<T, num_components> components) : components(components) {}

So you can pass by reference (to prevent extra copy). It will still need to be copied into the destination.

    Vector(std::array<T, num_components> const& components)
        : components(components)
    {}

So you can pass by r-value reference.

    Vector(std::array<T, num_components>&& components)
        : components(std::move(components()
    {}

Now some people may point out that std::array is non-movable. I know. But I like to future proof my code. At some point in the future somebody may change the type of components and I would like my code to still stay as efficient as possible. So it may be worth doing this just for the future. BUT I can also see the counter argument against so take or leave this one as you see fit.

Return by reference

Here you are returning by value.

    template<int idx>
    inline T Get() const {
        typename std::enable_if<(num_components >= idx)>::type();
        return this->components[idx];
    }

If all you want to do is read a member of T. Then making a copy (return by value causes a copy) then this seems overkill. Also if you return by reference there is the added benefit (if that is applicable) that you can potentially change the value in place. If you don't want to change the value then return a const reference to allow reads but not writes (via the reference).

inline is the most useless keyword.

The key word inline is a hint to the compiler that is universally ignored by all modern compilers. Humans are terrible at deciding when inlining is appropriate so the compilers started ignoring their human masters a long time ago and decide internally when to actually inline the code.

    inline T Get(int idx) const {

Only use the inline keyword when you have to. This is used when functions/methods are defined outside the class but in a header file (so there are potentially multiple copies across compilation units). Here it is used to tell the linker to ignore all the extra copies and they are the same.

Don't use this->

This is a code smell in C++ and hides errors.

    inline T GetX() const {
        return this->components[0];
    }

The only reason to use this-> is to disambiguify a shadowed member. The problem here is that if you forget to use this-> the compiler will not tell you there is an error it will use the most locally scoped version of the variable (so it hides errors when you forget to use it). Also for a bug fixer it is hard to tell if you deliberately did not use it and meant to use the local shadowing variable or you it was a mistake and you wanted the member variable.

On the other hand you only need it when you have shadowed variables. If you never have shadowed variables you never have to use it. You will also not have any ambiguity on which variable you meant to use because you used nice unique variables for everything.

Get/Set bad interface

    template<int idx>
    inline void Set(T value) {

I think the whole Java world got it wrong. Get/Set is a terrible paradigm for accessing an object as it breaks the encapsulation. What they did get very well is the whole automation of serialization and other tools that can be built when you do use this.

But it's also considered a bad pattern for C++ (we don't have any of that tooling (so our code is not so brittle to change).

Use references to avoid copying.

            const T value = this->components[i];
            ret += value * value;

Nothing wrong with breaking this into two lines and making it readable. But you have to watch the assignment to value. This is a copy operation. So you make a copy then multiply the values together.

OK a copy is not that bad for ints/doubles or any numeric types. But you are defining this for an arbitrary type T. The cost of copying T could potentially be huge so don't do it you don't need to.

            T const& value = this->components[i];  // Assign to reference
            //    ^^^ 
            ret += value * value;

You don't need move on return

        std::array<T, num_components> new_components;
        ...
        return this_t(std::move(new_components));

When you return a value it is a prvalue. So moving the object to a temporary is not required before a return (it will already be a prvalue) and the compiler will optimize and move a returned value.

    return new_components; // Achieves the same result.

Assignment vs Increment

These operators are all fine:

    this_t operator+(const this_t &r) const;
    this_t operator-(const this_t &r) const;
    this_t operator*(const this_t &r) const;
    this_t operator*(T s) const;

Some people may argue that these should be free standing functions. There is an argument for this if you want auto conversions to happen (which I usually don't). So I usually do as you have done and make them members. BUT you should look at your use case and make sure that is what you want.

Also when people define these methods they usually also define the:

    this_t& operator+=(const this_t &r) const;
    this_t& operator-=(const this_t &r) const;
    this_t& operator*=(const this_t &r) const;
    this_t& operator*=(T s) const;

This is because they are easy to write as a pair.

// The += looks a lot like your original code.
// except you are updating the current objet (so its not a const method)
this_t& operator+=(this_t const& r) {
    for (int i = 0; i < num_components; i++) {
        components[i] += r.components[i];
    }
    return *this;
}
// The + operator simply copies itself and then uses
// the += operator to do the hard work on the copy (which is returned).
this_t operator+(this_t const& r) const {
    this_t copy(r);
    return copy += r;
}

Its standard for the assignment to return a reference to itself.

This:

    bool operator=(const this_t &r) const;

Is normally written as:

    this_t& operator=(const this_t &r); // Can't be const as you are modifying this.
\$\endgroup\$
  • \$\begingroup\$ You confuse affine spaces with vector ones. A vector space of n finite dimensions over a field F is the set of ordered n-element sequences, not a set of pairs. An affine space is a set of points plus vectors tangent to that space. \$\endgroup\$ – bipll Oct 4 '18 at 10:03
0
\$\begingroup\$

There's static_assert to be used for that:

template<int idx>
inline T Get() const {
    static_assert(idx < num_components
        , "This vector does not have that many coordinates.");
    return this->components[idx];
}

And please note that inequality should be strict in assertions if components are zero-based.

\$\endgroup\$

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.