7
\$\begingroup\$

This is a continuation of An Alternative Vector, taking a closer look at the copy assignment operator.

In the following code, I am only posting the new version of the copy assignment operator; all the rest of the code is unchanged.

namespace ThorsAnvil
{
    // Traits classes
    //      SimpleCopyableAssignableTraitNoThrow: 
    //      SimpleDestructableTrivialy 
    // Helper classes
    //      SimpleDestructableTrivialy
    //      SimpleCopy

    // Forward declare the vector
    template<typename T>
    class Vector;

Declarations:

    template<typename T>
    struct SimpleCopyableAssignableTraitNoThrow
    {
        static constexpr bool value = std::is_nothrow_destructible<T>::value
                                &&    std::is_nothrow_copy_assignable<T>::value;
    };
    template<typename T>
    struct SimpleDestructableTrivialy
    {
        static constexpr bool value = std::is_trivially_destructible<T>::value;
    };
    template<typename T, bool = SimpleCopyableAssignableTraitNoThrow<T>::value>
    class SimpleCopy
    {
        public:
            void simpleCopy(Vector<T>& dst, Vector<T> const& src) const;
    };
    template<typename T, bool = SimpleDestructableTrivialy<T>::value>
    class SimpleDestroy
    {
        public:
            void destroyElements(Vector<T>& obj);
    };

Updated Vector

    template<typename T>
    class Vector
    {
        // The rest (see original question for details).

        friend class SimpleCopy<T>;
        friend class SimpleDestroy<T>;
        public:
            Vector& operator=(Vector const& value) noexcept(SimpleCopyableAssignableTraitNoThrow<T>::value)
            {
                SimpleCopy<T>   copier;
                copier.simpleCopy(*this, value);
                return *this;
            }
    };

SimpleDestroy

    template<typename T>
    class SimpleDestroy<T, true>
    {
        public:
            // The elements in object are trivially destructible.
            // This means nothing is done. As an optimization we
            // don't need to do anything to the elements, just
            // reset the size
            void destroyElements(Vector<T>& obj)
            {
                obj.length  = 0;
            }
    };
    template<typename T>
    class SimpleDestroy<T, false>
    {
        public:
            // destructor is non trivial but does not throw
            void destroyElements(Vector<T>& obj)
            {
                for(int loop = 0; loop < obj.length; ++loop)
                {
                    obj.buffer[obj.length - 1 - loop].~T();
                }
                obj.length = 0;
            }
    };

SimpleCopy

    template<typename T>
    class SimpleCopy<T, true>
    {
        public:
            // Neither constructor or destructor throw.
            // So we can optimize the copy.
            void simpleCopy(Vector<T>& dst, Vector<T> const& src) const
            {
                if (&dst == &src)
                {
                    return;
                }

                // Destroy the members of the current object
                SimpleDestroy<T>    destroyer;
                destroyer.destroyElements(dst);

                // Copy from the source object to the detonation
                // thus reusing the memory underneath. It only tries
                // to reallocate if the current object needs to be
                // re-sized.
                for(auto const& value: src)
                {
                    dst.push(value);
                }
            }
    };
    template<typename T>
    class SimpleCopy<T, false>
    {
        public:
            // Constructor or Destructor may fail.
            // So use Copy & Swap idiom to handle resource
            // allocation correctly.
            void simpleCopy(Vector<T>& dst, Vector<T> const& src) const
            {
                Vector<T>     tmp(src);       // Copy
                tmp.swap(dst);                // Swap
            }
    };           

}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Although there's a comment in SimpleDestroy<T, false>::destroyElements() claiming that ~T() doesn't throw, there's very little to assure that. Our only guarantee is that we currently call it only from the version of SimpleCopy that handles non-throwing destructors, but it would be easy to accidentally break that requirement - for example, we might want to use it to implement ~Vector<T>().

It's easy to make it more robust - decrement obj.length as we go along, rather than once at the end. That makes the behaviour more like standard containers.


I don't see why destroyElements() and simpleCopy() need their own (publicly-visible!) classes. Those classes could be private member classes of Vector, but I think that's still overkill: the functions may be better as private members of the Vector class:

private:
    void destroyElements_trivial();
    void destroyElements_nontrivial();
    void destroyElements() {
        (std::is_trivially_destructible_v<T>
            ? destroyElements_trivial : destroyElements_nontrivial)();
    }

    void simpleCopy_trivial(Vector<T> const& src);
    void simpleCopy_nontrivial(Vector<T> const& src);
    void simpleCopy(Vector<T> const& src) {
        (SimpleCopyableAssignableTraitNoThrow<T>::value
            ? simpleCopy_trivial : simpleCopy_nontrivial)(src);
    }

Implementation is straightforward:

template<typename T>
void Vector<T>::destroyElements_trivial()
{
    length  = 0;
}

template<typename T>
void Vector<T>::destroyElements_nontrivial()
{
    while (length) {
        buffer[--length].~T();
    }
    length = 0;
}


template<typename T>
void Vector<T>::simpleCopy_trivial(Vector<T> const& src)
{
    if (this == &src) {
        return;
    }

    // Destroy the members of the current object
    destroyElements();

    // Copy from the source object
    for (auto const& value: src) {
        push(value);
    }
}
template<typename T>
void Vector<T>::simpleCopy_nontrivial(Vector<T> const& src)
{
    Vector<T> tmp(src);       // Copy
    swap(tmp);                // Swap
}

We can further simplify into single methods, using if constexpr:

private:
    void destroyElements();
    void simpleCopy(Vector<T> const& src);
template<typename T>
void Vector<T>::destroyElements()
{
    if constexpr (!std::is_trivially_destructible_v<T>) {
        while (length) {
            buffer[--length].~T();
        }
    }
    length  = 0;
}


template<typename T>
void Vector<T>::simpleCopy(Vector<T> const& src)
{
    if (this == &src) {
        return;
    }

    if constexpr (SimpleCopyableAssignableTraitNoThrow<T>::value) {
        // Destroy the members of the current object
        destroyElements();

        // Copy from the source object
        for (auto const& value: src) {
            push(value);
        }
    } else {
        Vector<T> tmp(src);       // Copy
        swap(tmp);                // Swap
    }
}

That's cleaner and shorter than having the destruct and copy outsourced to friend classes. It also enables us to dispense with the traits classes (which can move to being static constexpr members), and yields this much reduced version of the question code:

#include <type_traits>

namespace ThorsAnvil
{
    template<typename T>
    class Vector
    {
        // copied from original question so it compiles
        std::size_t     capacity;
        std::size_t     length;
        T*              buffer;

        static constexpr bool is_nothrow_assignable =
            std::is_nothrow_destructible_v<T> && std::is_nothrow_copy_assignable_v<T>;
        static constexpr bool is_trivially_destructible =
            std::is_trivially_destructible_v<T>;

        public:
            Vector& operator=(Vector const& value) noexcept(is_nothrow_assignable)
            {
                simpleCopy(value);
                return *this;
            }

        T* begin() const;
        T* end() const;
        void push(T const& u);

    private:
        void destroyElements();
        void simpleCopy(Vector<T> const& src) noexcept(is_nothrow_assignable);
    };


    template<typename T>
    void Vector<T>::destroyElements()
    {
        if constexpr (is_trivially_destructible) {
            length = 0;
        } else {
            while (length) {
                buffer[--length].~T();
            }
        }
    }



    template<typename T>
    void Vector<T>::simpleCopy(Vector<T> const& src) noexcept(is_nothrow_assignable)
    {
        if (this == &src) {
            return;
        }

        if constexpr (is_nothrow_assignable) {
            // Destroy the members of the current object
            destroyElements();

            // Copy from the source object
            for (auto const& value: src) {
                push(value);
            }
        } else {
            Vector<T> tmp(src);       // Copy
            swap(tmp);                // Swap
        }
    }

}

Simple typos (more important than usual, given that this is didactic code):

  • Trivialy -> Trivially
  • detonation -> destination
\$\endgroup\$

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.