Smart pointer mark II - my answer to C++ 11 shared_ptr

Question

I am writing this smart pointer as a learning exercise. Any feedback would be most appreciated.

Any flaws? Have I missed any test cases?

smart_pointer.hpp:

#ifndef SMARTPOINTER_HPP_
#define SMARTPOINTER_HPP_
#include <iostream>
template<typename T>
class sp {
public:
 sp(T* ptr) : ptr_(ptr), ref_cnt_(new unsigned(1)) {
 std::cout << "sp ctor. Address: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
 }
 T& operator*() { return *ptr_; }
 const T& operator*() const { return *ptr_; }
 T* operator->() { return ptr_; }
 const T* operator->() const { return ptr_; }
 // access to raw ptr
 T* get() { return ptr_; }
 sp(const sp<T>& rhs) : ptr_(rhs.ptr_), ref_cnt_(rhs.ref_cnt_) {
 addref();
 std::cout << "sp copy ctor: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
 }
 sp& operator=(const sp& rhs) {
 if(this != &rhs) {
 try_delete();
 ptr_ = rhs.ptr_;
 ref_cnt_ = rhs.ref_cnt_;
 addref();
 std::cout << "sp assignment: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
 }
 return *this;
 }
 ~sp() { 
 std::cout << "~sp. Address: " << ptr_ << ", ref count before remove_ref: " << *ref_cnt_ << '\n';
 try_delete(); 
 }
private:
 unsigned addref() {
 return ++*ref_cnt_;
 }
 unsigned remove_ref() {
 return --*ref_cnt_;
 }
 void try_delete() {
 if(remove_ref() == 0) {
 std::cout << "try_delete(): finally deleting ptr_ at address: " << ptr_ << ", ref count: " << *ref_cnt_ << '\n'; 
 delete ref_cnt_;
 delete ptr_;
 }
 }
 T* ptr_; // raw ptr
 unsigned* ref_cnt_; // shared ownership
};
#endif //SMARTPOINTER_HPP_

Code to exercise:

#include <iostream>
#include <vector>
#include "smart_pointer.hpp"
// useless class just used to test with object more complex than int
class tester {
public:
 tester(const int id, const char* name) : id_(id), name_(name) {}
 const char* const get_name() const { return name_; }
 const int get_id() const { return id_; }
private:
 const int id_;
 const char* const name_;
};
// global vector to hold smart pointer
std::vector< sp<int> > gvs;
int main() {
 {
 std::cout << "Test with ints\n";
 sp<int> myptr1 = new int(3); 
 *myptr1 = 4;
 std::cout << "number: " << *myptr1 << '\n';
 {
 // test usage in collection
 std::vector< sp<int> > vs;
 vs.push_back(myptr1);
 }
 // global collection
 gvs.push_back(myptr1);
 // copying
 sp<int> myp2(myptr1);
 sp<int> myp3(myptr1);
 sp<int> myp4 = myptr1;
 }
 {
 std::cout << "Test with more complex objects\n";
 sp<tester> spt = new tester(1, "Keira Knightley");
 tester* p = spt.get();
 std::cout << "Name: " << p->get_name() << ", id: " << p->get_id() << '\n';
 // copying
 sp<tester> spt2 = spt;
 sp<tester> spt3(spt);
 sp<tester> spt4(new tester(2, "Cate Blanchett"));
 // re-assign
 spt = new tester(3, "Natalie Portman");
 }
 return 0;
}

• JaDogg: How about better class name ?

Answer 1

(1) Style nit: Prefer #pragma once to fragile #ifndef-based include guards. #pragma once isn't yet part of the standard; but any compiler that supports C++11 will definitely have supported #pragma once for years already.

(2) Obviously, remove the std::cout usage from your constructors when using this class for real. :)

(3) This is a MAJOR issue, and a common one, which is why I want to make a big deal of it:

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

No no no no no. You should provide only one operator*, and it should look like this:

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

The act of dereferencing a (smart) pointer does not modify the pointer object, and therefore should be a const method of that pointer object. Also, the result of dereferencing a (smart) pointer to T had darn well better be a reference to T! Adding const to the return type is just plain wrong.

This is the same confusion that people often have between int * const and int const *, or between std::vector<int>::iterator const and std::vector<int>::const_iterator. In your case the confusion is between sp<int> const and sp<int const>.

(4) As above, you should have only one operator-> and one get(), and both should be const methods:

T* operator->() const { return ptr_; }
T* get() const { return ptr_; }

(5) Your constructor should almost certainly be explicit:

explicit sp(T* ptr) : ptr_(ptr), ref_cnt_(new unsigned(1))

The exception-safety guarantees here look good to me. I would argue that size_t would be more appropriate than unsigned (you're probably paying for a 16-byte allocation anyway, so why not use that space?), but that's a nitpick.

If you really care about memory allocations, and want to keep learning about how the standard shared_ptr does things, you should try to implement the equivalent of std::make_shared for your smart pointer type, so that the memory allocations can be wrapped up together into a single call to new. I'd imagine that the tricky parts of make_shared have to do with alignment requirements.

(6) Your ref_cnt_ doesn't point to a std::atomic<unsigned>, so addref() is not atomic; copying the same sp from two threads at once may result in undefined behavior.

(7) In C++11, it would be advisable to implement a move constructor and a move assignment operator in order to avoid a pointless addref() and removeref() when moving an rvalue. However, there's no problem with correctness in C++11; because you implement a user-defined copy constructor, the compiler will correctly suppress the generation of default move constructor and assignment functions (which would do the Wrong Thing if they were generated, so it's a good thing they're not).

(8) Your copy assignment operator performs unnecessary operations in the case that x is being assigned from a copy of x; for example:

sp<int> x(new int);
sp<int> y = x; // adds a ref, correctly
x = y; // removes a ref, then adds a ref; could be implemented as a no-op instead

(9) You don't use remove_ref except inside try_delete. Personally, I would inline the one call to remove_ref, and then rename try_delete to remove_ref. I would also rename addref to add_ref for consistency, and remove its unused return value.

(Future) To continue building your smart pointer into something like the standard's std::shared_ptr, consider the following use-cases:

sp<int> s(nullptr); // or (int *)NULL
s = nullptr;
s.reset(); // same as =nullptr

Consider inheritance, upcasting and downcasting. This will require fleshing out your ref_cnt_ pointer from unsigned* to struct ref_cnt*, and careful consideration of what metadata you need to keep in there.

class Mother { int m; };
class Father { int f; };
class Child : public Mother, public Father { int c; };
sp<Child> ch(new Child);
sp<Father> fa = ch; // casting derived to base should work
ch.reset();
fa.reset(); // this should call ~Child on the correct pointer

Consider providing "weak references", which the standard library provides as std::weak_ptr. Weak references (weak_ptr) and strong references (shared_ptr) work tightly together, using the same infrastructure.

Consider providing the ability to pass in a custom deleter:

sp<char> buf(malloc(1024), (void(char*))free);
buf.reset(); // this should call free on the correct pointer