4
\$\begingroup\$

I have Implemented Unique_Ptr with move semantics for the sake of Learning. Any suggestions, feed backs are highly appreciated.

// author @asad_nitp
#pragma once

namespace Smart_Pointers {
template<typename T>

class Unique_Ptr {
    T* m_ptr;
public:
    explicit Unique_Ptr(T* ptr = nullptr) noexcept
        : m_ptr(ptr) 
    {}

    ~Unique_Ptr() noexcept {
        if (m_ptr != nullptr) {
            delete m_ptr;
        }
        m_ptr = nullptr;
    }

    Unique_Ptr(const Unique_Ptr&) = delete;
    Unique_Ptr& operator = (const Unique_Ptr&) = delete;

    Unique_Ptr(Unique_Ptr&& ob) noexcept {
        m_ptr = ob.m_ptr;
        ob.m_ptr = nullptr;
    }
    Unique_Ptr& operator = (Unique_Ptr&& ob) noexcept {
        if (this != &ob) {
            m_ptr = ob.m_ptr;
            ob.m_ptr = nullptr;
        }
        return *this;
    }

    T* operator -> () const noexcept {
        return m_ptr;
    }

    T& operator * () const {
        return *m_ptr;
    }

    T* get() const noexcept{
        return m_ptr;
    }

    T* release() noexcept {
        T* ans = m_ptr;
        m_ptr = nullptr;
        return ans;
    }

    void reset(T* ptr = nullptr) noexcept {
        T* old = m_ptr;
        m_ptr = ptr;
        if (old != nullptr)
            delete old;
    }

    void swap(Unique_Ptr& ob) noexcept {
        using std::swap;
        swap(m_ptr,ob.m_ptr);
    }

 };
} 
  //Examples
  int main() {
     Smart_Pointers::Unique_Ptr<int> up1(new int(5));
     Smart_Pointers::Unique_Ptr<int> up2(new int(6));
     up1 = up2; // Error
     up1 = std::move(up2); //fine

     return 0;
   }
\$\endgroup\$
  • 3
    \$\begingroup\$ At least one serios bug operator= will cause memory leak. Use swap. Use swap in the move constructor as well. delete shoudl be able to handle nullptr - no need for additional check \$\endgroup\$ – Artemy Vysotsky Sep 24 '17 at 12:52
5
\$\begingroup\$

As mentioned in the original comment you leak on assignment:

Unique_Ptr<int>   a = new int(1);
Unique_Ptr<int>   b;

a = b;  // You leak the original a pointer.

Looking at the assignment operator:

Unique_Ptr& operator = (Unique_Ptr&& ob) noexcept {
    if (this != &ob) {
        m_ptr = ob.m_ptr;     // Here you overwrite the current value.
                              // and thus leak its original value.
        ob.m_ptr = nullptr;
    }
    return *this;
}

As mentioned in the comments the standard way of implementing move semantics is:

Unique_Ptr(Unique_Ptr&& ob) noexcept
    : m_ptr(nullptr)
{
    swap(ob);
}
Unique_Ptr& operator = (Unique_Ptr&& ob) noexcept
{
    swap(ob);
    return *this;
}

This has a couple of benefits. Calling the destructor is not required. You are moving the actual data to the other object. Thus means the destructor of the other object will correctly handle destruction.

This also potentially allows for re-use. If the ob is in some other context and you the object could be re-used this can help in optimizations. If it can not be-reused it will correctly be deleted.

You remove the pesimizing check for self assignment.

You don't have a conversion too bool.

if (a) {
    // A is holding a value you can do stuff with it.
}

// Now you can do
if (a.get()) {
    // A is holding a value you can do stuff with it.
}
// But its not as neat. And the context changes.

This is simply fixed with:

// Note the explicit (added in C++11) to replace the safe-bool idiom
explicit operator bool() {
    return m_ptr;
}

Can you guarantee that the destructor of type T does not throw?

void reset(T* ptr = nullptr) noexcept;

I think not. I agree it should be but that is not something you can guarantee and thus your code should not guarantee that it does not. So this should probably be:

void reset(T* ptr = nullptr);

You can probably do some meta-programming to determine if the destructor is noexcept but I don't have that in my head.

Don't mark your destructor as noexcept.

~Unique_Ptr() noexcept; // this is not needed.
                        // Destructors are automatically noexcept

Your destructor is automatically noexcept unless specific conditions are met so the compiler will work it out correctly for you.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why is swap better than delete m_ptr; m_ptr = ob.m_ptr; ? \$\endgroup\$ – bigdata2 Apr 27 at 1:15
  • \$\begingroup\$ @bigdata2 The current implementation is broken (as shown above). The standard pattern is to use swap. Now you could do it manually as you describe (with a few more statements as it is a move). But you should also implement the swap() methods as well so you may as well use it and there is no difference in cost. Now I mentioned that using swap() is the standard way of doing it. So by maintaining the standard pattern you can re-use the same technique in the future. \$\endgroup\$ – Martin York Apr 27 at 4:15
  • \$\begingroup\$ I was referring to the assignment operator, where there was a memory leak. I see an issue with swap -- after swapping, the onus is on the programmer to call a delete on the pointer, which can result in a leak if delete is not called. \$\endgroup\$ – bigdata2 Apr 27 at 4:40
  • \$\begingroup\$ @bigdata2 There is no leak when using swap(). You swap the current pointer into the object being passed in. That object has a destructor which will destroy the pointer in it. \$\endgroup\$ – Martin York Apr 27 at 5:32
  • \$\begingroup\$ @bigdata2 I go into a lot of detail in a blog post I wrote here: part1 part3 \$\endgroup\$ – Martin York Apr 27 at 5:34

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.