Discussion on: What's unique pointer in C++

View post

The destructor is overly complex :P delete nullptr is a no-op by standard and, after the destruction, the value can't be accessed anymore, so it could just be:

~unique_ptr() {
  delete m_ptr;
}

Gapry • Jul 9 '20 • Edited

Hi, Ranieri Althoff

I agree with you the destructor is overly complex since the expression m_ptr = nullptr; is redundant.

Assume we design the destructor as you describe, someone call the ~unique_ptr() twice or above. The destructor of m_ptr will be called not only once, hence we need to check whether m_ptr is nullptr.

Finally, the design of the destructor should be as following description.

~unique_ptr() {
  if (m_ptr) {
    delete m_ptr;
  }   
}

Thank you for your rely.

Best regards, Gapry.

Dave Cridland • Jul 12 '20

If someone is calling your destructor twice, you have bigger problems - and m_ptr won't be set to nullptr if it's called twice.

Also, you want to delete the implicit copy constructor (otherwise you'll have multiple owners), and similarly with the assignment operator - and you need a move constructor and a move-assignment operator to make the whole thing more useful (and conformant).

Gapry • Jul 12 '20 • Edited

Hi, Dave Cridland

Thank you for your suggestion and insights. I think the similar design may be like this.

class noncopyable
{
public:
  noncopyable() = default;
  ~noncopyable() = default;

  noncopyable(const noncopyable& rhs) = delete;
  noncopyable& operator=(const noncopyable& rhs) = delete;
};

template <class T>
class unique_ptr : public noncopyable  {
  T* m_ptr = nullptr;

public:
  unique_ptr (unique_ptr&& rhs)
    : m_ptr(std::move(rhs.m_ptr)) 
  {} 

  unique_ptr& operator=(unique_ptr&& rhs) {
    std::move(rhs.m_ptr);
    return *this;
  }
};

Thank you for your rely.

Best regards, Gapry.

Ranieri Althoff • Jul 12 '20

How is it possible to call a destructor twice if that's an unique pointer? Double destructor call wouldn't impliy that you are sharing the pointer?

Gapry • Jul 13 '20 • Edited

Hi, Ranieri Althoff

Assume we're in the multi-threading and the environment is Linux. The m_ptr points to the shared-memory which normally is the heap in Linux.

auto task(void) -> void {
  auto pt = beta::unique_ptr<point<float>>(new point<float>(1.2f, 2.3f, 3.4f, 4.5f));
  // skip the left detail implementation.
}

auto worker1 = std::thread(task);
auto worker2 = std::thread(task);
auto worker3 = std::thread(task);

For now, we have three threads need to be executed and their have the same task. If we don't check the m_ptr, we call the destructor not only once.

I find a picture that you may be interested so I want to shared it with you. The picture sources from here.

Thank you for your rely.

Best regards, Gapry.

Dave Cridland • Jul 13 '20

No, because the point instances are unique in each thread.

If you put the following into the task lambda, you'll see:

  // ...
  std::cout << &pt << ' ' << &*pt << std::endl;
  // ...

That will show different pointer values for all three unique pointers, and the objects they point to.

You literally cannot (implicitly) call a destructor more than once without some really esoteric and low-level behaviour (such as using the clone() system call on Linux to fork into a thread).

Gapry • Jul 13 '20 • Edited

Hi, Dave Cridland

Agree!! You're right !!!! I'm wrong, sorry.

If I change the code as following, what do you think?

auto task(point* pt_) -> void {
  auto pt = beta::unique_ptr<point<float>>(pt_);
  // skip the left detail implementation.
}
auto pt = new point<float>(1.2f, 2.3f, 3.4f, 4.5f);
auto worker1 = std::thread(task, pt);
auto worker2 = std::thread(task, pt);
auto worker3 = std::thread(task, pt);
// skip the left detail implementation.

Thank you for your rely.

Best regards, Gapry.

Dave Cridland • Jul 13 '20

Right! If you do that, you will get a double-free, and essentially nothing can protect you (because the m_ptr of each unique_ptr is independent). But also, you don't need to use threads to do it - and it's why best practise with smart pointers is to use std::make_unique:

auto pt = std::make_unique<point<float>>(1.2f, 2.3f, 3.4f, 4.5f);

Gapry • Jul 13 '20

Hi, Dave Cridland

Thank you for your rely, insights and suggestion!

Best regards, Gapry.