-1

I'm trying to make a sample implementation of std::unique_ptr. Here is the code:

include <iostream>

template<typename T>
class UniquePtr
{
public:
  explicit UniquePtr(T * ptr)
    :m_ptr(ptr)
  {};

  UniquePtr(UniquePtr const & other) = delete;
  UniquePtr& operator=(UniquePtr const & other) = delete;

  explicit UniquePtr(UniquePtr && other)
  {
    m_ptr = other.m_ptr;
    other.m_ptr = nullptr;
  }

  UniquePtr & operator=(UniquePtr && other)
  {
    std::cout << "Move assignment called " << std::endl;
    m_ptr = other.m_ptr;
    other.m_ptr = nullptr;
  }

  ~UniquePtr()
  {
    delete m_ptr;
  }

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

  T& operator->()
  {
    return m_ptr;
  }

private:
  T * m_ptr = nullptr;
};

int main()
{
  UniquePtr<int> t(new int(3));
  t= UniquePtr<int>(new int(4));

  std::cout << *t << std::endl;
}

This code compiles and I'm able to see the value 4 in the output even after deleting the default assignment and copy constructor. What am I doing wrong?

Aman Gupta
  • 537
  • 1
  • 7
  • 22
  • `t= UniquePtr(new int(4));` is a `move` in this case. – ChrisMM Apr 17 '21 at 14:29
  • Related: https://stackoverflow.com/questions/3106110/what-is-move-semantics/11540204#11540204 – ChrisMM Apr 17 '21 at 14:31
  • What you're doing wrong is not returning `*this` from your move assignment operator. That's the only thing that you are doing wrong. Can you explain why you believe that the move constructor is getting called? – Sam Varshavchik Apr 17 '21 at 14:33
  • @ChrisMM, that's what I also thought. But why is that? Also what happens to the pointer which is pointed to by the first unique ptr? – Aman Gupta Apr 17 '21 at 14:38
  • @SamVarshavchik, because that line gets printed. I added `return *this` but I still get the same result – Aman Gupta Apr 17 '21 at 14:38
  • The only line that prints anything, in the shown code, is the line in the move ***assignment*** operator. Not the move ***constructor***. Nothing gets printed in the move constructor. Pop quiz: what is `operator=`? Is it: 1) the move constructor, or 2) the move assignment operator? – Sam Varshavchik Apr 17 '21 at 14:40
  • Please read the linked page. The answer on it actually discusses `unique_ptr` example, including what happens to the `new int(3)` (it gets leaked) – ChrisMM Apr 17 '21 at 14:40
  • I didn't quite get your question. You're using a move assignment operator. So, why don't you expect a 4 in the ouptut? – Zoso Apr 17 '21 at 14:48

1 Answers1

0

In the assignment, move is called because the UniquePtr<int>(new int(4)) is constructing a temporary object, and in this case the compiler tries to use the move assignment if possible, else it would fall back to the copy assignment, which is deleted.

UniquePtr<int> t(new int(3));
t= UniquePtr<int>(new int(4)); // move assignment because temporary
auto k = t; // copy assignment since t is not temporary and so does not compile.

As commented, the move assignment is not returning *this, you should enable all warnings. Also the last operator-> has a syntax error, it returns a pointer but expects a reference.

Additionally, your code has a major issue, in case of exceptions you could have a memory leak. Suppose you write something like:

class Banana
{ ... }

void eatBanana(UniquePtr<Banana> banana, int amountToEat);
int computeAmount();

eatBanana(UniquePtr<Banana>(new Banana(3)), computeAmount());

If, for any reason, the computeAmount() function throws an exception, then the memory allocated by new could never be released, because computeAmount() could be executed between new and the constructor of the UniquePtr. For this reason, we normally use std::make_unique().

You should implement your own version of make_unique() and use it, it's trival, see here: https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique

Here more information about the issues and the solution: https://www.oreilly.com/library/view/effective-modern-c/9781491908419/ch04.html

Remy Lebeau
  • 454,445
  • 28
  • 366
  • 620
andreaplanet
  • 671
  • 4
  • 13
  • 1
    You should clarify that `auto k = t;` won't compile. – Zoso Apr 17 '21 at 14:50
  • it is a temporary object but the object still gets leaked, right? is there anyway to avoid that? Maybe this is not a real-world use-case anyway.. – Aman Gupta Apr 17 '21 at 14:51
  • *If for any reason the constructor of Banana throws an exception, then the memory allocated by new will never be released* is incorrect. The leak scenario is a bit more nuanced, q.v. https://stackoverflow.com/a/37514601/4641116 – Eljay Apr 17 '21 at 15:15
  • Yes, just after writing the example I realized it wasn't correct, fixed quickly. – andreaplanet Apr 17 '21 at 15:21