2
\$\begingroup\$

Please verify whether the code for a move constructor and a move assignment operator is correct.

#include <iostream>
using namespace  std;

class MyVector {
    size_t m_Size;
    int* m_pInt;
public:`enter code here`
    ~MyVector()
    {
        delete[] m_pInt;
    }

    MyVector():m_Size(0), m_pInt(nullptr)
    {   }

    MyVector(int x)
    {
        m_Size = x;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = i;
        }
    }

    void print() const
    {
        for (int i = 0; i < m_Size; ++i)
        {
            cout << m_pInt[i] << endl;
        }

    }

    MyVector(const MyVector& rhs)
    {
        m_Size = rhs.m_Size;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = rhs.m_pInt[i];
        }
    }



    MyVector(MyVector&& rhs):m_Size(move(rhs.m_Size)), m_pInt(move(rhs.m_pInt))
    {
        rhs.m_Size = 0;
        rhs.m_pInt = nullptr;
    }


    MyVector& operator=(const MyVector& rhs)
    {
        if (this != &rhs)
        {
            if (m_pInt != nullptr)
            {
                delete m_pInt;
            }
            m_Size = rhs.m_Size;
            m_pInt = new int[m_Size];
            for (int i = 0; i < m_Size; ++i)
            {
                m_pInt[i] = rhs.m_pInt[i];
            }
        }
        return *this;
    }

    MyVector& operator=(MyVector&& rhs)
    {
        if (this != &rhs)
        {
            if (m_pInt != nullptr)
            {
                delete m_pInt;
            }
            m_Size = move(rhs.m_Size);
            m_pInt = move(rhs.m_pInt);
            rhs.m_Size = 0;
            rhs.m_pInt = nullptr;
        }
        return *this;
    }
};

int main()
{
    MyVector ob1(10);
    // Copy  
    MyVector ob2;
    ob2 = ob1; // Asssignment operator
    MyVector ob3 = ob1; // Copy Ctr

    ob1.print();
    // Move 
    MyVector ob4(move(ob1));
    MyVector ob5;
    ob5 = move(ob1);



    ob1.print();

    return 0;

}
\$\endgroup\$

closed as off-topic by jonrsharpe, forsvarir, Mast, alecxe, Edward Apr 2 '17 at 17:36

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – jonrsharpe, Mast, Edward
  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – forsvarir, alecxe
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ This isn't that kind of code review; take the tour, see How to Ask. If you aren't certain it works, test it. \$\endgroup\$ – jonrsharpe Apr 2 '17 at 8:54
5
\$\begingroup\$

Default constructor

    MyVector():m_Size(0), m_pInt(nullptr)
    {   }

Stop trying to save space like that. THe point of good coding is to make it readable by humans. The computer can read any style so try and make it more maintainable by making it readable.

I would write it like this:

    MyVector()
        : m_Size(0)
        , m_pInt(nullptr)
    {}

Sure you can use nullptr for the zero sized array. But this seems like a premature optimization. You can make the rest of your code less complicated by never having a null m_pInt. If there is never a null then your code does not need to check for it.

Also currently you have different behavior for:

MyVector  x;
MyVector  x(0);

Constructor

    MyVector(int x)
       // Use the initializer list to initialize the elements.
    {
        m_Size = x;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = i;
        }
    }

Print

Sure this works. But std::cout is not the only stream you way want to print too

    void print() const
    {
        for (int i = 0; i < m_Size; ++i)
        {
            cout << m_pInt[i] << endl;
        }
    }

So I would pass a stream to print(). It can default to std::cout. Then you should add an operator<<. As the normal way of printing something is via <<.

  MyVector  d(15);
  std::cout << d;

Note: prefer "\n" to std::endl. The difference is a stream flush. The stream will already flush itself when required. So you adding extra flushes is only going to cause the flushes to be less optimal.

Copy Constructor

    MyVector(const MyVector& rhs)
        // Use the initializer list
    {
        m_Size = rhs.m_Size;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = rhs.m_pInt[i];
        }
    }

Move constructor

    MyVector(MyVector&& rhs):m_Size(move(rhs.m_Size)), m_pInt(move(rhs.m_pInt))
    {
        rhs.m_Size = 0;
        rhs.m_pInt = nullptr;
    }

Yes that works. But you should also mark the move constructor as noexcept. The standard containers have optimizations that can be applied if they know tour class will not throw when being moved. Otherwise they have to fallback to copying to provide the strong exception guarantee.

The standard technique is to use swap though. It makes it look and behave just like move assignment. See below.

Copy Assignment

Yes the test for self assignment looks like a good optimization.

    MyVector& operator=(const MyVector& rhs)
    {
        if (this != &rhs)
        {

BUT its not. Self assignment happens so rarely (in fact basically never in real code) that what you are doing is pesimizing the normal flow and as a result will make your code slower. You do need to cope with self assignment but because it is so rare you don't need to worry that it is not the optimal path.

The second issue I have here is that you destroy the local data before you have a copy of the new data ready.

            if (m_pInt != nullptr)
            {
                delete m_pInt;
            }

If something goes wrong in the rest of your code then you will be unable to roll back the state and thus can not provide the strong exception guarantee. When copying an object it should happen in three distict phases.

1. Copy the source into a temporary object.                  This is dangerous as exceptions can happen. So you must do this without changing the state of the current object.
2. Swap the state of the current object with the temporary.  Swapping is supposed to be exception safe. So you can safely change the state of your object.
3. Now you can release the old resources.                    This is unlikely to go wrong; But even if something goes wrong your object is in a good state.

So your Copy assignment should look like this:

MyVector& operator=(const MyVector& rhs)
{
    // Make a copy to temp
    std::size_t tSize = rhs.m_Size;
    int*        tInt  = new int[tSize];
    std::copy(rhs.m_pInt, rhs.m_pInt + rhs.m_Size, tInt);

    // Now swap with your current object state.
    std::swap(tSize, m_Size);
    std::swap(tInt,  m_pInt);

    // now destroy the old data;
    delete [] tInt;

    // Done
    return *this;
}

If you look carefully at those three stages. Stage 1 looks like the constructor and stage 3 looks like the destructor and stage 2 looks like a standard swap function so we can simplify the above to exactly that:

MyVector& operator=(const MyVector& rhs)
{
    MyVector  temp(rhs);        // 1. Copy
    temp.swap(*this);           // 2. Swap

    // Done
    return *this;
}                               // 3. delete done by destructor.

This is called the copy and swap idiom.

Move Assignment

Again the pesimizing test for self assignment.

    MyVector& operator=(MyVector&& rhs)
    {
        if (this != &rhs)

The standard move assignment is to swap the source and the destination. This has a couple of benefits.

  1. You don't call delete (and thus don't invoke the destructor). Thus it is potentially faster.
  2. Because you did not delete the data there is an opportunity for it to be reused.
  3. If the source is going out of scope it will invoke its destructor and destroy your data but it will be done after the completion of your object thus giving you strong exception guarantee. Thus allowing you to make your assignment operator noexcept.

Standard Move Assignment

MyVector& operator=(MyVector&& rhs) noexcept
{
    rhs.swap(*this);
    return *this;
}

I wrote a series of posts about all this.

Vector - Resource Management Allocation
Vector - Resource Management Copy Swap
Vector - Resize
Vector - Simple Optimizations
Vector - the Other Stuff

Re-Write

class MyVector {
    size_t m_Size;
    int*   m_pInt;
public:
    ~MyVector()
    {
        delete[] m_pInt;
    }

    MyVector(int x = 0)
        : m_Size(x)
        , m_pInt(new int[m_Size])
    {
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = i;
        }
    }

    void print(std::ostream& str = std::cout) const
    {
        for (int i = 0; i < m_Size; ++i)
        {
            str << m_pInt[i] << "\n";
        }
    }
    friend std::ostream& operator<<(std::ostream& str, MyVector const& data)
    {
        data.print(str);
        return str;
    }

    MyVector(const MyVector& rhs)
        : m_Size(rhs.m_Size)
        , m_pInt(new int[m_Size])

    {
        std::copy(rhs.m_pInt, rhs.m_pInt + rhs.m_Size, m_pInt);
    }

    MyVector(MyVector&& rhs) noexcept
        : m_Size(0)
        , m_pInt(nullptr)
    {
        rhs.swap(*this);
    }

    MyVector& operator=(MyVector const& rhs)
    {
       MyVector  t(rhs);
       t.swap(*this);
       return *this;
    }

    MyVector& operator=(MyVector&& rhs) noexcept
    {
        rhs.swap(*this);
        return *this;
    }

    void swap(MyVector& other) noexcept
    {
        using std::swap;
        swap(m_Size, other.m_Size);
        swap(m_pInt, other.m_pInt);
    }
    friend void swap(MyVector& lhs, MyVector& rhs)
    {
        lhs.swap(rhs);
    }
};
\$\endgroup\$
  • \$\begingroup\$ Hi. What do you mean by "Stop trying to save space like that." regarding the code MyVector():m_Size(0), m_pInt(nullptr) { }? What would you write instead if this was the only constructor? \$\endgroup\$ – krismath May 1 '17 at 21:20
  • 2
    \$\begingroup\$ @krismath: One initialization per line. The point is to write readable code not try and squeeze as many operations onto a single line as you can. Updating answer with how I would write it. \$\endgroup\$ – Martin York May 1 '17 at 23:03
  • \$\begingroup\$ MyVector() could be noexcept if you allow for m_pInt being nullptr. That's worth the hassle easily. \$\endgroup\$ – Deduplicator May 1 '17 at 23:16
  • 1
    \$\begingroup\$ @Deduplicator Not sure a noexcept default constructor buys you anything. Are there any optimizations in the STL this will enable? If it does not gain you anything then why add the extra complexity to the rest of your code. Especially when it adds two different meanings for basically the same declaration MyVector x; /* nullptr but size 0*/ and MyVector y(0); /* Not nullptr but size 0*/ \$\endgroup\$ – Martin York May 1 '17 at 23:31
  • \$\begingroup\$ Fantastic answer. 1. I wonder why didn't you add m_pInt = nullptr in the destructor after deleting it. Omission? 2. In the ctor: MyVector(int x = 0) : m_size(0). I believe you wanted to say m_size(x) here right? 3. I always tend to use {} instead of () for initialization of objects because it prevents widening and is more readable (doesn't look like a function call). \$\endgroup\$ – Nikos Oct 19 '18 at 3:47

Not the answer you're looking for? Browse other questions tagged or ask your own question.