4
\$\begingroup\$

I've made a templated math vector struct with a templated type and templated dimension count.

I want my vectors to be convertible so I can easily make, for example, a vec<int, 3> from a vec<float, 3>

I want to do it in a safe way, or at least I'd like to know the ways it's unsafe.

I've never used static_cast or dynamic_cast and I've never used the operator user-defined conversion.

Here is what I came up with (only the relevant parts of the struct):

template<class t, size_t dimensions> struct vec{
    typedef t coord_type;       
    static constexpr size_t dimension_count = dimensions;

    std::array<coord_type, dimension_count> coords;

    template<class new_coord_type> operator vec<new_coord_type, dimension_count>() const{
        std::array<new_coord_type, dimension_count> casted_coords;
        std::transform(
            coords.begin(),
            coords.end(), 
            casted_coords.begin(),
            [](coord_type coord){return static_cast<new_coord_type>(coord);}
        );
        return {casted_coords};
    }
};

It seems to work how I want it to. For example, this compiles and gives me the correct results:

vec<float, 3> v1{1.1f, 2.5f, 3.9f};
vec<int, 3> v2 = v1; // converted correctly to {1, 2, 3}

The only way I could figure out how to cast the contents of an array is to use std::transform. I'm not sure if there's a better way.

Final questions: Is this unsafe or incorrect? How can this be improved?

\$\endgroup\$
  • \$\begingroup\$ If you are dealing with this type of mathematical vector, you may want to look at std::valarray \$\endgroup\$ – Justin Aug 2 '17 at 19:07
3
\$\begingroup\$

I had to adjust your code style a bit before I could easily read the code. In particular:

  • Use InitialCaps for template parameters
  • Add a space before {
  • A newline after template<...> generally improves readability

Thus:

template<class T, size_t Dimensions>
struct vec {
    using coord_type = T;       
    static constexpr size_t dimension_count = Dimensions;

    std::array<T, Dimensions> coords;

    template<class U>
    operator vec<U, Dimensions>() const {
        std::array<U, Dimensions> casted_coords;
        std::transform(
            coords.begin(),
            coords.end(), 
            casted_coords.begin(),
            [](T coord) {
                return static_cast<U>(coord);
            }
        );
        return {casted_coords};
    }
};

Notice that I personally prefer to use the original template parameters T, Dimensions, etc. in places where your code used the member typedefs coord_type, dimension_count etc.; I find that this improves readability but I believe reasonable people may differ on the subject.

(In particular, the C++ Standard always uses member typedefs in declarations such as reference operator*() const as opposed to T& operator*() const; but I think that's because the name reference is part of the API of the class, whereas the name T is given for exposition only. That's a Standard-ism that we don't necessarily need to emulate when writing our own code.)


Nit: Dimensions is technically the wrong name for a single dimension. Also, by capitalizing the template parameter, we've freed up the shorter name dimensions (or dimension) for the constexpr member variable. You never have to use a tediously long name like dimension_count if you manage your naming real estate effectively!


        return {casted_coords};

The extra braces here are a pessimization; they inhibit move-elision (a.k.a. Named Return Value Optimization). Remove them... oh wait, I see, casted_coords is a std::array, not a vec! Why are you constructing a separate array and then copying it into the vec? That seems pointless!

At the very least, this should be

return { std::move(casted_coords) };

but ideally you'd define casted_coords to be of type vec<U, Dimensions> and then transform the data straight into casted_coords.coords.begin().


            [](T coord) {
                return static_cast<U>(coord);
            }

This lambda takes its parameter by-copy, which is going to be expensive if T is, say, std::string. Prefer to take generic parameters by const& or (in C++14) by perfect-forward:

            [](const T& coord) {
                return static_cast<U>(coord);
            }

            [](auto&& coord) {
                return static_cast<U>(std::forward<decltype(coord)>(coord));
            }

In this case the former is better because it's less typing (and thus fewer chances to screw something up).


Your operator vec<U, Dimensions>() should be explicit, unless you have a darn good reason for it not to be. Most possible reasons I can think of do not qualify as "darn good."


In fact, I'd prefer to go further, and say that you shouldn't be using C++'s native "conversion" mechanisms for this kind of conversion at all. IMO you should write a user-defined function, along the lines of static_pointer_cast or any_cast, that does precisely and explicitly what you want.

template<class To, class From, size_t Dim>
auto static_vec_cast(const vec<From, Dim>& from) -> vec<To, Dim> {
    vec<To, Dim> to;
    std::transform(
        from.coords.begin(),
        from.coords.end(), 
        to.coords.begin(),
        [](const From& x) {
            return static_cast<To>(x);
        }
    );
    return to;
}

int main() {
    vec<int, 3> a = {{1,2,3}};
    vec<float, 3> b = static_vec_cast<float>(a);
}

Season with SFINAE to taste.


However, this still has a major flaw: You're accidentally requiring that To be default-constructible! That's no good (especially if it does happen to be default-constructible but the default constructor is expensive). You can fix this issue, but it requires metaprogramming.

template<class To, class From, size_t Dim, size_t... Is>
auto static_vec_cast(const vec<From, Dim>& from, std::index_sequence<Is...>) -> vec<To, Dim> {
    vec<To, Dim> to = {{
        static_cast<To>(from.coords[Is])...
    }};
    return to;
}

template<class To, class From, size_t Dim>
auto static_vec_cast(const vec<From, Dim>& from) -> vec<To, Dim> {
    return static_vec_cast<To>(from, std::make_index_sequence<Dim>{});
}

The advantage of this approach is that it's hardly any more lines of code, but it's more correct, and it doesn't rely on running std::transform at runtime — it just generates the correct code directly inline.

Writing an efficient static_vec_cast<U>(vec<T, Dim>&&) is left as an exercise for the reader.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a lot. I find your answer very insightful. I don't think I disagree with anything. Answering in order, I had the typedefs because I wanted the template types to be members similar to std::array::value_type, but maybe it isn't' necessary. I like your coding style. I agree it's more readable. Idk why I'm not completely conforming to the standard library style, but I want to from now on. I love the idea of static_vec_cast. It reminds me of the std::chrono::duration_cast. That is what I'll try. :) \$\endgroup\$ – Willy Goat Aug 2 '17 at 18:08
  • \$\begingroup\$ Just one more point: This is one of the situations where ignoring "prefer composition over inheritance" pays huge dividends. \$\endgroup\$ – Deduplicator Aug 2 '17 at 18:24
  • 1
    \$\begingroup\$ @Deduplicator: FWIW, I probably disagree (depending on what's in the part of the code the OP didn't show us); inheritance almost always sucks. For example, I doubt the OP wants operator< to work the same way it does for vectors; which means he's got to figure out the right syntax for overriding or =deleteing it. OTOH, if all the OP wanted was the static_vec_cast function, then he won't need struct vec at all and can get by with plain old std::vector — no composition and no inheritance! :) \$\endgroup\$ – Quuxplusone Aug 2 '17 at 22:09

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.