4
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ How about better class name ? \$\endgroup\$ Feb 17 '15 at 16:27
3
\$\begingroup\$

(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
\$\endgroup\$

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.