6
\$\begingroup\$

It has been a long time I used c++.
So I brushing over concepts for interview preparation.
Can you please help me with code review?
If you have any observations/bugs/alternatives please point out.
For better formatting, I have shared code here feel free to open issues. :)
Thanks a ton in advance.

#include <cstring>
#include <memory>
#include <utility>

class Test {
public:
    //Default constructor
    Test() = default;

    //Copy constructor
    Test(const Test& other) : m_size(other.m_size), m_buffer(new char[m_size]()) {
        strncpy(m_buffer.get(), other.m_buffer.get(), m_size);
    } 

    //Parametarized constructor
    Test(std::size_t len, const char* data) : m_size(len), m_buffer(new char[len]()) {
        strncpy(m_buffer.get(), data, m_size);
    }

    //Move constructor
    Test(Test&& other) noexcept {
        Test temp(other);
        swap(*this, temp);
    }

    //Assignment operator
    Test& operator=(Test other) {
        if(this != &other) {
            swap(*this, other);
        }
        return *this;
    }

    //Move assignment operator
    Test& operator=(Test&& other) noexcept {
        if(this != &other) {
            Test temp(other);
            swap(*this, temp);
        }
        return *this;
    }

    //Destructor
    ~Test() {
        m_size = 0;
        m_buffer = nullptr;
    }
private:
    std::size_t m_size = 0;    
    std::unique_ptr<char[]> m_buffer = nullptr;

    static void swap(Test& first, Test& second) noexcept{
        using std::swap;
        swap(first.m_size, second.m_size);
        swap(first.m_buffer, second.m_buffer);
    }
};
\$\endgroup\$
5
\$\begingroup\$

Observations

Your rule of 5 implementation is basically wrong. You are implement copy operations for both copy and move (which means you may as well not have the move operations).

Your destructor is not doing any real work and thus adding unneeded code.

Your swap function does not work as expected as it can not be used with Koenig lookup (ADL) and thus in most standard library would be replaced with std::swap resulting in a sub optimal swap.

Details

Here I don't see the need for creating a temporary.

    //Move constructor
    Test(Test&& other) noexcept {
        Test temp(other);
        swap(*this, temp);
    }

By creating the temporary your move is basically just as expensive as your copy. The whole point of the move is that it is cheap.

I would simply swap the current with other.

    //Move constructor
    Test(Test&& other) noexcept {
        swap(other);
    }

After the move construction the content of other has no guaranteed state (only that it is valid). Since you define m_size and m_buffer to auto initialize the current this is in a valid state and can simply be swapped with other.

    // Note I usually define a `noexcept` swap method.

    void swap(Test& other) noexcept {
        using std::swap;
        swap(m_size,   other. m_size);
        swap(m_buffer, other. m_buffer);
    }

I also define swap() with two parameters as a free standing function that simply calls the swap method. This is because the compiler can using Koenig lookup (ADL) to find the correct swap function.

    void swap(Test& lhs, Test& rhs) {
        lhs.swap(rhs);
    }

The Copy assignment is sub optimal.

    //Assignment operator
    Test& operator=(Test other) {
        if(this != &other) {
            swap(*this, other);
        }
        return *this;
    }

Since the parameter other is created via a copy construction it is guaranteed not to be the same as this. Thus the test if(this != &other) is just a pessimization as it is not needed.


Again you perform a copy during the move assignment.

    //Move assignment operator
    Test& operator=(Test&& other) noexcept {
        if(this != &other) {
            Test temp(other);
            swap(*this, temp);
        }
        return *this;
    }

This defeats the purpose of the move (as it is supposed to be cheaper than a copy). You can simply swap this and other as they should both be valid.

    Test& operator=(Test&& other) noexcept {
        swap(other);
        return *this;
    }

No need to check for self assignment as this is a pessimization of the normal more common situation. Even of they are the same object the swap will work correctly.


The destructor is useless:

    //Destructor
    ~Test() {
        m_size = 0;
        m_buffer = nullptr;
    }

The member: m_buffer will correctly clean up the memory.


Making this a static member is not doing you any favors as it prevents Koenig lookup (ADL).

    static void swap(Test& first, Test& second) noexcept{
        using std::swap;
        swap(first.m_size, second.m_size);
        swap(first.m_buffer, second.m_buffer);
    }

Make this a free standing function:

namespace PL
{

class TestStatic
{
    public:
        static void swap(TestStatic& lhs, TestStatic& rhs) noexcept
        {}
};


class TestFreeStand
{
    public:
};
void swap(TestFreeStand& lhs, TestFreeStand& rhs) noexcept
{}

}


int main()
{
    PL::TestStatic      staticA;
    PL::TestStatic      staticB;

    swap(staticA, staticB);


    PL::TestFreeStand   freeStandA;
    PL::TestFreeStand   freeStandB;

    swap(freeStandA, freeStandB);
}

Notice the only compilation error here is: swap(staticA, staticB);

This means that if you add using std::swap; this code will now compile but it will not use your static void swap() method it will use std::swap which makes a copy of the object. The point of writing your own version of swap is that you are providing an optimization over the standard swap.

Better Implementation

#include <cstring>
#include <memory>
#include <utility>

namespace Testing
{

    class Test
    {
        public:
            // Default constructor
            Test() = default;
        
            // Parametrized constructor
            Test(std::size_t len, const char* data)
                : m_size(len)
                , m_buffer(new char[len]())
            {
                // Should validate the input here.
                // It is the only time you get outside input.
                // Potentially you could throw here I choose
                // not too (because I can see it as a valid input)
                // Your use case may be different.
                if (data) {
                    strncpy(m_buffer.get(), data, m_size);
                }
            }
    
            // Copy constructor
            Test(const Test& other)
                : m_size(other.m_size)
                , m_buffer(new char[m_size]())
            {
                strncpy(m_buffer.get(), other.m_buffer.get(), m_size);
            }     
        
            // Copy Assignment operator
            Test& operator=(Test other)
            {
                swap(other);
                return *this;
            }
    
            // Move constructor
            Test(Test&& other) noexcept
            {
                swap(other);
            }
        
            // Move assignment operator
            Test& operator=(Test&& other) noexcept
            {
                swap(other);
                return *this;
            }
        
            void swap(Test& rhs) noexcept
            {
                using std::swap;
                swap(m_size,    rhs.m_size);
                swap(m_buffer,  rhs.m_buffer);
            }
        private:
            std::size_t m_size = 0;    
            std::unique_ptr<char[]> m_buffer = nullptr;
    };
    
    void swap(Test& lhs, Test& rhs) noexcept
    {
        lhs.swap(rhs);
    }
}
\$\endgroup\$
1
\$\begingroup\$

Your move-ctor and move-assignment are badly implemented. You copy input data and then you swap it with source. You may as well delete move-ctor and move-assignment as they are worthless now.

Your copy assignment operator has an unnecessary check that cannot be true and it won't perform well for self-copying. As it will copy its data into another instance and then swap data with it. To fix this make input of type const Test&. And make a data copy routine.

Destructor is unnecessary as std::unique_ptr automatically manages it.

You realize strncpy copies data till it reaches 0, right?

   std::unique_ptr<char[]> m_buffer = nullptr;

This is unnecessary. No need for = nullptr. std::unique_ptr does it on its own.

Edit: Also I believe declaring a private static swap is a problem. What will happen if user were to try to call swap(tst1,tst2) when using namespace std or using std::swap?

You could make it public, but IMHO it's better to implement move-ctor and move-assignment directly - without making a swap in between. Let swap rely on move and not the opposite. Note that std::swap utilizes move for its purposes.

\$\endgroup\$
10
  • \$\begingroup\$ I agree with for copy assignment related part as I am accepting arg by value. But for move ctor and move assignment, it is recommended to use copy-swap idiom ref. stackoverflow.com/a/3279550/10035556 \$\endgroup\$
    – Mayur
    May 24 '20 at 9:46
  • \$\begingroup\$ Let me know your thought on copy-swap idiom or a better alternative for the same :) \$\endgroup\$
    – Mayur
    May 24 '20 at 9:51
  • \$\begingroup\$ @Mayur for move-ctor and move-assignment operations, you didn't implement them correctly. They are meant to steal input's data. Your implementation doesn't. It copies the data. \$\endgroup\$
    – ALX23z
    May 24 '20 at 11:15
  • \$\begingroup\$ @Mayur the copy-swap idiom is pretty bad honestly. Frequently, I want to reserve memory allocation and reuse it while keeping reserved size. Swapping ruins it. Just make a reserve/resize functions that ensures that enough data is allocated for copying and increases the capacity appropriately. \$\endgroup\$
    – ALX23z
    May 24 '20 at 11:24
  • 1
    \$\begingroup\$ @ALX23z Sure. But in the general case you can't get away from that if you want to implement the strong exception guarantee. In situations like the above were you can get away without the extra allocation it is simple to implement the conditional copy and swap. Also that is why we have the move constructor. So you don't need to allocate memory. These are not new concepts and we (as a community) have done excessive testing on all these things. In the end micro optimizations like you suggest are not worth the extra maintenance cost. That's why its an idiom. \$\endgroup\$ May 24 '20 at 22:55

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.