Is it ok to dynamic cast "this" as a return value?

Question

This is more of a design question.

I have a template class, and I want to add extra methods to it depending on the template type. To practice the DRY principle, I have come up with this pattern (definitions intentionally omitted):

template <class T>
class BaseVector: public boost::array<T, 3>
{
protected:
    BaseVector<T>(const T x, const T y, const T z);
public:
    bool operator == (const Vector<T> &other) const;
    Vector<T> operator + (const Vector<T> &other) const;    
    Vector<T> operator - (const Vector<T> &other) const;
    Vector<T> &operator += (const Vector<T> &other)
    {
        (*this)[0] += other[0];
        (*this)[1] += other[1];
        (*this)[2] += other[2];

        return *dynamic_cast<Vector<T> * const>(this);
    }

    virtual ~BaseVector<T>()
    {
    }
}

template <class T>
class Vector : public BaseVector<T>
{
public:
    Vector<T>(const T x, const T y, const T z)
    : BaseVector<T>(x, y, z)
    {
    }
};

template <>
class Vector<double> : public BaseVector<double>
{
public:
    Vector<double>(const double x, const double y, const double z);
    Vector<double>(const Vector<int> &other);
    double norm() const;
};

I intend BaseVector to be nothing more than an implementation detail. This works, but I am concerned about operator+=. My question is: is the dynamic cast of the this pointer a code smell? Is there a better way to achieve what I am trying to do (avoid code duplication, and unnecessary casts in the user code)? Or am I safe since, the BaseVector constructor is private?

EDIT:

Sorry, yes I have virtual dtor, but I forgot to paste it, the code doesn't compile without it.

Answer 1

I would recommend that you consider an alternative approach (note that in the below examples, I have simplified the code to the bare minimum required to demonstrate the alternative approaches).

First, consider the Curiously Recurring Template Parameter (CRTP):

template <typename T, typename DerivedVector>
struct BaseVector
{
    DerivedVector& operator+=(DerivedVector const& other)
    {
        return static_cast<DerivedVector&>(*this);
    }
};

template <typename T>
struct Vector : BaseVector<T, Vector<T>>
{
};

Since you always know what the derived type is, a static_cast is sufficient. If Vector<T> is the only class whose base is BaseVector<T> and if you are guaranteed that the T parameters are always the same, then strictly speaking, the CRTP parameter is unnecessary: you always know what the derived type is, so a static_cast is safe.

Alternatively, consider that operators need not be member functions, so you could declare non-member operator function templates:

template <typename T, typename DerivedVector>
struct BaseVector
{
};

template <typename T>
struct Vector : BaseVector<T, Vector<T>>
{
};

template <typename T>
Vector<T>& operator+=(Vector<T>& self, Vector<T> const& other)
{
    return self;
}

While the latter is preferable for operators, it won't work for ordinary, nonoperator member functions, so the CRTP approach would be preferable for those.

Answer 2

Yes, from a technical point of view it is OK. However, in order for the dynamic_cast to work your base class needs to be polymorphic. So you need to make at least the destructor (pure) virtual.

I also want to add that instead of:

// potential null dereference
return *dynamic_cast<Vector<T> * const>(this);

it is safer to write:

// potential std::bad_cast exception
return dynamic_cast<Vector<T> & const>(*this);

To answer your original question:

Is there a better way to achieve what I am trying to do (avoid code duplication, and unnecessary casts in the user code)?

Yes. Read about static polymorphism, curiously recurring template pattern and policy-based class design if you want to learn more.

Answer 3

None of your methods are virtual. Without any virtual methods, the compiler won't add any run-time type information. Without RTTI, dynamic_cast will not work.

Answer 4

I think a better way to accomplish this is the pimpl idiom. As you say, BaseVector is just an implementation detail. As such, clients of your class should have no knowledge of it (which leaves you free to change it).

In this case, this would entail having Vector contain a BaseVector rather than inherit from it. It would then define its own arithmetic assignment operators, which would forward to BaseVector.

This does not violate DRY, because there is still only one version of the implementation details, and they reside in BaseVector. Repeating the interface is perfectly fine.

Answer 5

This is an old question, but I recently had to solve this same problem and then happened to stumble across this, so I thought I'd answer.

I used a type wrapper in order to have the specialisation 'derive from itself':

template <class T>
struct type_wrapper;

template <class T>
struct unwrap_type
{
    typedef T type;
};

template <class T>
struct unwrap_type<type_wrapper<T>>
{
    typedef T type;
};

template <class WrappedT>
class Vector: public boost::array<typename unwrap_type<WrappedT>::type, 3>
{
    typedef typename unwrap_type<WrappedT>::type T;
protected:
    Vector(const T x, const T y, const T z);
public:
    bool operator == (const Vector &other) const;
    Vector<T> operator + (const Vector &other) const;    
    Vector<T> operator - (const Vector &other) const;
    Vector<T> &operator += (const Vector &other)
    {
        (*this)[0] += other[0];
        (*this)[1] += other[1];
        (*this)[2] += other[2];

        return static_cast<Vector<T> &>(*this);
    }
}

template <>
class Vector<double> : public Vector<type_wrapper<double>>
{
public:
    Vector(const double x, const double y, const double z);
    Vector(const Vector<int> &other);
    double norm() const;
};

That way, you don't need any redundant classes - except for the type wrapper and metafunction, which are reusable.