Easy misuse in `cxx::unique_ptr`

[elfenpiff]

Required information

Since the deleter is part of the constructor argument but otherwise unchangeable it is extremely hard to track which deleter is currently in use.

Do you spot the bug:

cxx::unique_ptr<int> a(new int(1), [](auto*ptr) {delete ptr;});
cxx::unique_ptr<int> b(custom_alloc.new<int>(1), [](auto * ptr) { custom_alloc.free(ptr); });

a = std::move(b);
auto old_int_ptr = a.release(new int(2));

The deleter changes silently when b is moved to a but the user may assumes that a is still a unique_ptr which calls delete on destruction. This can be mixed up extremely easily!

The solution would be to make the deleter part of the unique_ptr type like the STL does. The deleter would be a callable type which releases the underlying resource and would look like:

template<typename T>
struct CallDelete {
  operator()(T*ptr) { delete ptr; }
};

cxx::unique_ptr<int, CallDelete> a(new int(1));
cxx::unique_ptr<int, CustomDelete> b(custom_alloc.new<int>(1));

a = std::move(b); // compile failure ... bug from above cannot happen.

[mossmaurice]

@elfenpiff I never understood why cxx::unique_ptr was implemented like it is today in the first place. IIRC @ithier wanted to store the same cxx::unique_ptrs with different deleters in a container, which wouldn't directly be possible with std::unique_ptr due to the different template instantiations. Not sure if that is still needed today. We could use a cxx::variant as a wrapper if it's still needed.

[elBoberido]

@elfenpiff @mossmaurice having the deleter as part of the unique_ptr does not fully solve the problem. In our case, one still could call the wrong subscriber to release a sample.

// pseudocode
UntypedSubscriber s1;
UntypedSubscriber s2;

auto sample = unique_ptr<T>(static_cast<T*> s1.take(), [&](ptr) { s1.release(); });
sample.reset(static_cast<T*> s2.take());

To make this safe, the reset method must require a deleter.