c++constructorcopy-constructorobject-reference

Is it okay to move around references to avoid heavy copy constructor calls?


Let's say, we want to reverse an array, like in this function.

For each swapping of two elements, the copy constructor, destructor and two copy assignments are made.

template<class T>
void Reverse(T *arr, int size)
{
    T *leftItem = arr;
    T *rightItem = &arr[size - 1];

    for (int i = 0; i < size / 2; i++, leftItem++, rightItem--)
    {
        T swap = *leftItem;         // Copy constructor
        *leftItem = *rightItem;     // Copy assignment
        *rightItem = swap;          // Copy assignment
    }                               // Destructor
}

This seems horrific. So I came up with an idea and I'm not sure whether it's for the better or worse, since I'm practically bypassing all type safetly and conventions here.

But on the other hand, it avoids copy operations that, depending on the type, could be heavy.

template<class T>
void ReverseUsingMemcpy(T *arr, int size)
{
    char *swap = new char[sizeof(T)];
    T *leftItem = arr;
    T *rightItem = &arr[size - 1];

    for (int i = 0; i < count / 2; i++, leftItem++, rightItem--)
    {
        memcpy(swap, (char*)leftItem, sizeof(T));
        memcpy((char*)leftItem, (char*)rightItem, sizeof(T));
        memcpy((char*)rightItem, swap, sizeof(T));
    }

    delete[] swap;
}

So, is my approach suitable, if the type of <T> might be any class type? What are the benefits, or what are the reasons to really avoid this? Is there a preferred way to move around the contents of structs without breaking anything?

Note: I'm aware of the std::vector type, but I want to also understand references and type-safety more.


Solution

  • Your proposed solution is safe only for trivially-copyable types (that is, those that do not have a user-defined copy constructor or copy-assignment operator).

    Even when dealing with trivially-copyable types, there's no reason to use memcpy in this case. It's less readable and no more performant than simply using the assignment operator.

    Consider a very simple (useless) example where your proposed solution would fail:

    struct SelfRef
    {
        SelfRef* self_;
    
        SelfRef() : self_{this} {}
        SelfRef(const SelfRef&) : self_{this} {}
        SelfRef& operator=(const SelfRef&) { return *this; }  // no-op
    };
    

    Copying such an object via memcpy would directly copy the self_ field, bypassing the copy constructor or copy-assignment operator and result in self_ pointing to a different location than this. Demo.

    In general, copy constructors and copy assignment operators exist to maintain the invariants of your classes when they're copied. Bypassing the assignment operator of a class that has one can and will quickly lead to those invariants being violated and the assumptions that your classes make about their state becoming incorrect.


    The correct way to swap two objects is to use an appropriate swap function that is appropriately optimized for the type being swapped. The usual way to do this is to pull std::swap into the local namespace and let ADL choose a better swap function if possible:

    template<class T>
    void Reverse(T *arr, int size)
    {
        T *leftItem = arr;
        T *rightItem = &arr[size - 1];
    
        for (int i = 0; i < size / 2; i++, leftItem++, rightItem--)
        {
            using std::swap;
            swap(*leftItem, *rightItem);
        }
    }
    

    Demo

    That way, you can provide a swap function specialized for your type in the same namespace as that type and it will get found by ADL while standard library types, built-in types, and types without an optimized swap function will fall back to std::swap (which may also have optimized versions for different categories of types).

    You could also use std::iter_swap, which implements those same semantics for you when using iterators:

    template<class T>
    void Reverse(T *arr, int size)
    {
        T *leftItem = arr;
        T *rightItem = &arr[size - 1];
    
        for (int i = 0; i < size / 2; i++, leftItem++, rightItem--)
        {
            std::iter_swap(leftItem, rightItem);
        }
    }
    

    Demo


    Of course, in the real world you should just use std::reverse instead of rolling your own Reverse function at all.