C++ Simple Shared Pointer Implementaion

Question

I wrote an implementation of a shared pointer. I would like a review of it.

It seems to work, but running it through Valgrind shows that that it leaks memory somewhere in my tests, but I don't know where.

Here is my .h file:

#include <stdexcept>
//Try to create a smart pointer? Should be fun.
template <class T>
class rCPtr {
 T* ptr;
 T** ptrToPtr;
 long* refCount;
public:
 rCPtr()
 {
 try
 {
 ptr = new T;
 ptrToPtr = &ptr;
 refCount = new long;
 *refCount = 1;
 }
 catch (...)
 {
 if (ptr)
 delete ptr;
 if (refCount)
 delete refCount;
 }
 }
 explicit rCPtr(T* pT)
 {
 try
 {
 ptr = pT;
 ptrToPtr = &ptr;
 refCount = new long;
 *refCount = 1;
 }
 catch(...)
 {
 if (ptr)
 delete ptr;
 if (refCount)
 delete refCount;
 }
 }
 ~rCPtr()
 {
 (*refCount)--;
 if (*refCount <= 0)
 {
 delete ptr;
 ptr = nullptr;
 delete refCount;
 refCount = nullptr;
 }
 }
 // I wonder if this is even necessary?
 bool exists() const {
 return *ptrToPtr; // Will be null if already freed (SCRATCH THAT IT CRASHES)
 }
 //Copy
 rCPtr(const rCPtr& other) : ptr(other.ptr), refCount(other.refCount)
 {
 (*refCount)++;
 }
 // If you pass another of same object.
 rCPtr& operator=(const rCPtr &right)
 {
 if (this == &right) {return *this;}
 T* leftPtr = ptr;
 long* leftCount = refCount;
 ptr = right.ptr;
 refCount = right.refCount;
 (*refCount)++;
 (*leftCount)--;
 // delete if no more references to the left pointer.
 if (*leftCount <= 0)
 {
 delete leftPtr;
 leftPtr = nullptr;
 delete leftCount;
 leftCount = nullptr;
 }
 return *this;
 }
 // This will create a 'new' object (call as name = new var) Make sure the type matches the one used for this object
 rCPtr& operator=(const T* right)
 {
 if (right == ptr) {return *this;}
 T* leftPtr = ptr;
 long* leftCount = refCount;
 ptr = right;
 *refCount = 1; // New refCount will always be 1
 (*leftCount)--;
 if (*leftCount <= 0)
 {
 delete leftPtr;
 leftPtr = nullptr;
 delete leftCount;
 leftCount = nullptr;
 }
 }
 T* operator->() const
 {
 if (exists())
 return *ptrToPtr;
 else return nullptr;
 }
 T& operator*() const
 {
 if (exists())
 return **ptrToPtr;
 // I dont know what else to do here
 throw std::out_of_range("Pointer is already deleted");
 }
 // Gives ref to ptr
 // Pls don't try to delete this reference, the class should take care of that
 const T& get() const
 {
 return *ptr; // This reference does not count to refCount
 }
 // if used in bool expressions if(rCPtr) {I think}
 explicit operator bool() const
 {
 return *ptrToPtr;
 }
 // returns the number of references
 long getRefCount() const
 {
 return *refCount;
 }
 // Will attempt to cast the stored pointer into a new type & return it
 // You probably should not delete this one either
 template <class X>
 X* dCast()
 {
 try
 {
 // X must be poly morphic or else, the cast will fail and cause an compiler error.
 auto casted = dynamic_cast<X*>(*ptrToPtr);
 return casted;
 }catch(const std::bad_cast& me)
 {
 return nullptr;
 }
 }
 // Resets current instance, if other instances exist will not delete object
 void reset()
 {
 (*refCount)--;
 ptrToPtr = nullptr;
 if (*refCount <= 0) // If the amount of references are 0 (negative count may explode ??)
 {
 delete ptr;
 ptr = nullptr;
 delete refCount;
 refCount = nullptr;
 }
 }
};
//Notes just finished typing it up. It compiled so that is good
//Valgrind DOES NOT like it though

I will take the chance to say thank you to anyone who answers this

/ Edit: Thanks for everyone who helped with answering my questions! (And for putting up with all of them as well)

• Mr R: HI @Ragov - surely if you have a refCount you use that for exists (don't need the ptrToPtr then). AND the dCast gives a un-counted pointer so is risky - you want to keep people using your class not going raw pointers.
• Ragov: @MrR Thanks! In your opinion, should the d cast return another rCPtr or what?
• Mast: "It compiled so that is good" That's good, yes, but did you test whether it actually produces the correct output when used? Have you tested your work?
• Ragov: @Mast I did do a quick test, that probably was not enough, but it did give me the right output for it (So, I guess my IDE/compiler did something to fix it behind the scenes)
• pacmaninbw: I was going to suggest that you add the unit test cases to the code, however, that may invalidate the answer. To those in the Close Vote Queue, the code works, helping someone find memory leaks is part of what we do here on code review.

Answer 1

Data members

I don't see the point of the T** ptrToPtr member variable. Every instance of *ptrToPtr can be replaced by ptr. Remove that member and you have one less thing to delete.

(削除) Why is refCount a pointer to a long? If you make is just a long then you don't need to delete it. (削除ここまで) I see what's happening. Every instance that has the same pointer needs to have access to the same refCount. Got it.

Now I can see your thinking with ptrToPtr. Remember that pointers only contain addresses. If you copy a pointer, both pointers refer to the exact same data. You don't need to share references to pointers. That's why the member ptrToPtr is not needed. A pointer and a copy of a pointer do just as well pointing to the same data. From now on, consider it removed from the class.

Methods

Default constructor: rCPtr()

The default constructor allocates for a new default-constructed T and increments the refCount to one. Why? There's no data to be stored. There's no reason to call new when the user hasn't asked for anything to be stored. A better version would be

rCPtr() : ptr(nullptr), refCount(nullptr)
{
}

This will also make your exists() method work properly.

Constructor with pointer argument: explicit rCPtr(T* pT)

First, good job using explicit. That's good default for single-argument constructors.

Here, the only line that can possible throw is refCount = new long;. So, just as in the default constructor, there's no need for a try block.

rCPtr(T* pT) : ptr(pT), refCount(pT ? new long(1) : nullptr)
{
}

First, notice that you can combine a new statement with an initialization (new long(1)). As much as possible, create variables with the final values in the same statement that creates them.

There's no need for a try block here. If new long throws an exception (that is, your program tries and fails to allocate space for a single long) then your program (or your whole computer) is about to crash, so there's no reason to try to recover from this.

Notice the different initialization value of the refCount variable. If this instance contains a nullptr, then refCount should be a nullptr like in the default constructor.

NEW: Many commenters disagree with me about not checking for new long failing, so here's a version that handles that exception:

rCPtr(T* pT) try : ptr(pT), refCount(pT ? new long(1) : nullptr)
{
}
catch
{
 delete pT;
 throw;
}

This class is expected to handle the memory that the pointer points to, so the catch block deletes the pointer. Otherwise, the user would have to surround every rCPtr constructor with a try-catch block, which defeats the purpose of the class. You could also consider throwing a different exception like a std::runtime_error to inform other parts of the program what specifically failed.

Destructor: ~rCPtr()

This method is exactly the same as reset(). So, let's reuse the method.

~rCPtr()
{
 reset();
}

You'll want to reuse reset() as much as possible because it contains the most important code: that which releases resources when the last rCPtr is destructed. It's so important that you should only write it once and get that one time right.

Check for non-null pointer: bool exists() const

This function can just return ptr. A nullptr will convert to false, anything else to true. This works correctly with the fixed constructors.

Copy constructor: rCPtr(const rCPtr& other)

Your copy constructor is fine. Although, the member ptrToPtr was not copied or restored with ptrToPtr = &ptr. So, copied rCPtrs have an uninitialized pointers that crash the program (or, worse, return garbage data) upon dereference. But, the ptrToPtr is no more, so that's not a problem any more.

Assignment operator with rCPtr argument: rCPtr& operator=(const rCPtr &right)

Consider this alternative to the assignment operator:

rCPtr& operator=(const rCPtr &right)
{
 if(ptr == right.ptr) { return *this; }
 reset();
 ptr = right.ptr;
 refCount = right.refCount;
 (*refCount)++;
 return *this;
}

You don't care the previous state of the rCPtr instance before the assignment, so just call reset() to release the previous pointer. If the pointer is the same as the one in the argument, the pointer will not be deleted since the argument also contains a reference to it, so the reference count cannot reach zero.

Assignment operator with a pointer: rCPtr& operator=(const T* right)

Convince yourself that this alternative to the assignment operator has the same behavior as your code:

rCPtr& operator=(const T* right)
{
 if(right == ptr) { return *this; }
 reset();
 ptr = right;
 refCount = new long(1);
 return *this;
}

As in the previous assignment operator, you don't care the previous state of the rCPtr instance before the assignment, so just call reset() to release the previous pointer.

One other problem: I don't think this should compile. The method assigns a const T* to a T*. This should not be allowed because the data the argument points to is const. Are you compiling with -permissive?

Also, I added the missing return *this;.

Getting the underlying data: const T& get() const

This method's signature doesn't match the get() method in standard C++ smart pointers. The get() method in std::shared_ptr and std::unique_ptr return a raw pointer: T*. So, your method should be

T* get() const
{
 return ptr;
}

NEW: Returned pointer should be non-const.

Dereferencing: T* operator->() const

In your code now, if the pointer is not nullptr return the pointer, otherwise, return a nullptr. This is equivalent to just returning the pointer return ptr;. Or, return get();. It's good to reuse methods so that work happens in very few places that are easier to keep track of.

Dereferencing: T& operator*() const

(削除) First, this should not compile. The method returns a non-const reference from a const method. This would allow the data pointed to by the pointer to be modified from a const rCPtr. This does not seem right. There should be two methods for dereferencing the pointer: const T& operator*() const and T& operator*(). (削除ここまで) What you had is correct. This method being const is the same as dereferencing a const pointer. The pointer cannot change, but the data it points to certainly can unless that data is also const.

So, after replacing **ptrToPtr with *ptr, we need to consider what happens when exists() returns false. Right now, your code throws an exception. This is a valid choice. The other choice that is taken by std::unique_ptr and std::shared_ptr is to just call *ptr regardless and crash the program if it is a nullptr. This is also valid and puts the user on notice that they are responsible for not doing that. Your way is safer, the other way can be faster. Either choice is fine.

Checking for non-null pointer: explicit operator bool() const

(削除) Two (削除ここまで) One thing. (削除) First, explicit does not belong here as it is only used for constructors with one argument. Second, (削除ここまで) You can call return exists(); here to reuse that method.

NEW: I just learned that the explicit keyword can be used with conversion operators to prevent implicit conversions.

Get reference count: long getRefCount() const

This method won't work after calling reset() since refCount is deleted. We need to check for exists() before dereferencing a nullptr.

long getRefCount() const { return exists() ? *refCount : 0; }

Casting: template <class X> X* dCast()

Interesting method. But, casting to a pointer returns nullptr if the cast is unsuccessful. std::bad_cast is only thrown if the cast is to a reference type. So this method can be reduced to

template <class X>
X* dCast()
{
 return dynamic_cast<X*>(ptr);
}

Deleting data: void reset()

The all-important method. After, removing the **ptrToPtr statement, the method needs to get rid of any references to the pointed-to data. Even if there are other references, this instance needs to stop referring to them. So, the pointers are now set to nullptr unconditionally.

void reset()
{
 if( ! exists()) { return; }
 (*refCount)--;
 if (*refCount <= 0) // If the amount of references are 0 (negative count may explode ??)
 {
 delete ptr;
 delete refCount;
 }
 ptr = nullptr;
 refCount = nullptr;
}

With some of the changes above, we have to make sure that refCount is not nullptr before check its dereferenced value. One consistency check for this class is to make sure that either both ptr and refCount are nullptr or neither are.

Your comment tells me that you are afraid that a negative refCount means that something has gone wrong. You are right about that. Studying and testing your code will be necessary to avoid this situation, as it probably means that delete will soon be called on already deleted data.

Next things to think about

This code works for single-threaded programs, but what happens if two instances of rCPtr pointing to the same data (*refCount == 2) are in different threads of a program and reset() is called on both at the same time? That is, what happens when each line of reset() is called simultaneously in both instances in parallel? Shared pointers require a std::mutex at the beginning of the reset() function to make sure that multithreaded operation does not cause problems (there are other ways as well).

• Mathieu Guindon: Comments are not for extended discussion; this conversation has been moved to chat.

Answer 2

Code Review

Don't see the point in this member:

 T** ptrToPtr;

---

 rCPtr()
 {
 try
 {
 ptr = new T;

Calling new to create a new T is not appropriate here (having an empty object is perfectly fine). As the user may turn around and set a new pointer to own. As a result you don't need a try/catch block on this one.

I don't mind the way you use try/catch here but it is worth pointing out the try/catch for constructors can be written to include the initializer list.

 rCPtr()
 : ptr(nullptr)
 , refCount(nullptr)
 {}
 

---

Yes you do need the try block on this constructor (because you have guaranteed that the object will be managed and thus destroyed). Note this destruction is not just about calling delete and memory management but also about doing the correct resource management.

 explicit rCPtr(T* pT)
 {
 try
 {
 ptr = pT;
 ptrToPtr = &ptr;
 refCount = new long; // Why not allocate and initialize
 *refCount = 1; // in one statement?
 }
 catch(...)
 {
 if (ptr) // Don't need to check for null before
 delete ptr; // calling delete.
 if (refCount) // If the new long throws this is in
 delete refCount; // an indeterminate state. So calling
 // delete here is actually UB.
 // If there is an exception you need to make sure that
 // it continues.
 // otherwise your object contains invalid data by
 // making the exception continue you are saying that the
 // construction failed and the object is not valid.
 throw; // re-throws the current exception.
 }
 }

Though this try/catch is fine, I like the special version of try/catch for constructors as it allows you put the try/catch around the initializer list.

 explicit rCPtr(T* pT)
 try
 : ptr(pT)
 , ptrToPtr(&ptr)
 , refCount(new long(1))
 {}
 catch(...)
 {
 delete ptr;
 // don't need to call delete on refCount.
 // this is the only thing that could have failed and thrown
 // so the value is in an indeterminate state anyway,
 // Note: rethrow is not needed here.
 }

---

I have an issue with checking if refCount is "smaller" or equal to zero. If your refCount has gone below zero when you don't expect it to then you have some serious bug in your application (you have probably already called delete on a non-null pointer and thus this is likely to be a second call to delete on the pointer and you have just entered UB territory).

If your ref count does something unexpected the only thing you can do is exit the application and hope that nothing serious has been destroyed by the data.

 ~rCPtr()
 {
 (*refCount)--;
 // If refCount is smaller than z
 if (*refCount <= 0)
 {
 delete ptr;
 ptr = nullptr; // nulling out ptr is a waste of time.
 // It also hides errors. The debug
 // standard libraries have extra code for
 // doing memory validation.
 //
 // If you have a bug and don't reset the
 // ptr to null then the debug libraries
 // may be able to help you detect it.
 //
 //
 // There is a small possibility that the previous delete throws.
 // you still want to make sure that the next delete works
 // even if the previous one throws.
 delete refCount;
 refCount = nullptr;
 }
 }

---

You only set two of your three member variables. Make sure constructors always initialize all members.

 rCPtr(const rCPtr& other) : ptr(other.ptr), refCount(other.refCount)
 {
 (*refCount)++;
 }

---

Note: They are the same if they both use the same refCount object.

 // If you pass another of same object.
 rCPtr& operator=(const rCPtr &right)
 {
 // I would check if refCount == right.refCount
 // If they are the same then you are not going to change.
 if (this == &right) {return *this;}
 T* leftPtr = ptr;
 long* leftCount = refCount;
 ptr = right.ptr;
 refCount = right.refCount;
 (*refCount)++;
 (*leftCount)--;
 // delete if no more references to the left pointer.
 if (*leftCount <= 0)
 {
 delete leftPtr;
 leftPtr = nullptr;
 // Again there is a small chance that the above delete will throw.
 // You need to make sure that this second delete is called even
 // if that happens.
 delete leftCount;
 leftCount = nullptr;
 }
 return *this;
 }

Personally I would still use the copy-and-swap idiom to implement this functionality. It is a tried and tested technique to give you the strong exception guarantee.

 rCPtr& operator=(const rCPtr &right)
 {
 rCPtr copy(right);
 swap(copy);
 return *this;
 // This way the destructor handles all the deallocation and ref
 // counting, and you keep that all in the same space.
 //
 // Note: Here it is the destructor of `copy` that will make sure
 // the object ref counts are all kept correctly aligned.
 }

---

 rCPtr& operator=(const T* right)
 {
 // If you assign a pointer that is already being managed.
 // That is a major bug in your user's code.
 // Not sure ignoring it is the correct solution.
 //
 // Not sure what the best answer is either.
 if (right == ptr) {return *this;}
 T* leftPtr = ptr;
 long* leftCount = refCount;
 ptr = right;
 // THIS IS A BUG.
 // This refCount is still pointing at the refCount
 // object for the original value. This value may be
 // shared with several other smart pointers so you can
 // not reuse it. You should allocate a new one.
 //
 // If you have several objects that were using the refCount
 // object they all now have a refCount of 1 which may result
 // in your counter going below zero.
 *refCount = 1; // New refCount will always be 1
 // Note: leftCount and refCount still both point at the
 // same object. This means the counter just reached
 // zero. So the next section is going to deallocate it.
 (*leftCount)--; 
 if (*leftCount <= 0)
 {
 delete leftPtr;
 leftPtr = nullptr;
 // Same issue with the delete as before.
 // This sort of emphasizes that common code should be
 // written once and in its own function. That way if you
 // find a bug you only have to fix it in one place and
 // that remoes the human errror of fixes that same bug
 // in multiple pleaces.
 delete leftCount;
 leftCount = nullptr;
 }
 }

---

In the standard version of smart pointers, calling * or -> is UB if the contained pointer is null. It is the responsibility of the caller to make sure that the smart pointer is holding an appropriate value.

 T* operator->() const
 {
 if (exists())
 return *ptrToPtr; // Indentation issue.
 else return nullptr;
 } 
 T& operator*() const
 {
 if (exists())
 return **ptrToPtr; // Indentation issue.
 throw std::out_of_range("Pointer is already deleted");
 }

---

Note dynamic_cast<> when applied to pointers does not throw (it will return a null pointer on failure).

 // Will attempt to cast the stored pointer into a new type & return it
 // You probably should not delete this one either
 template <class X>
 X* dCast()
 {
 try
 {
 // X must be polymorphic, or else the cast will fail and cause a compiler error.
 auto casted = dynamic_cast<X*>(*ptrToPtr);
 return casted;
 }catch(const std::bad_cast& me)
 {
 return nullptr;
 }
 }

---

I wrote a couple of articles on this stuff:

Unique Ptr
Shared Ptr
Specialized constructors

---

How I would write it:

#include <stdexcept>
//Try to create a smart pointer? Should be fun.
template <class T>
class rCPtr
{
 template<typename U>
 class DeletePtrUtility
 {
 U* ptr;
 public:
 DeletePtrUtility(U* p) : ptr(p){}
 ~DeletePtrUtility() {delete ptr;}
 };
 T* ptr;
 long* refCount;
 public:
 rCPtr()
 : ptr(nullptr)
 , refCount(nullptr)
 {}
 explicit rCPtr(T* pT)
 try
 : ptr(pT)
 // Only allocate a refCount if the pointer is not null
 // this matches the default constructor action.
 , refCount(pT ? new long(1) : nullptr)
 {}
 catch(...)
 {
 delete pT;
 }
 ~rCPtr()
 {
 // This is the only place where delete happens.
 // So make sure it is correct. All other operations
 // that can result in a delete will do this be invoking
 // the destructor (using copy and swap).
 if (refCount)
 {
 // If the pointer stored is null then the refCount
 // is also null so we can't de-reference it.
 --(*refCount);
 if (*refCount == 0)
 {
 // Make sure delete is always called on both
 // pointers (even if there is an exception).
 DeletePtrUtility<T> deleteDataPtr(ptr);
 DeletePtrUtility<long> deleteRefCPtr(refCount);
 }
 }
 }
 void swap(rCPtr& other)
 {
 std::swap(ptr, other.ptr);
 std::swap(refCount, other.refCount);
 }
 friend void swap(rCPtr& lhs, rCPtr& rhs)
 {
 lhs.swap(rhs);
 }
 rCPtr(const rCPtr& other)
 : ptr(other.ptr)
 , refCount(other.refCount)
 {
 if (refCount) {
 // If there is an object, increment refcount.
 ++(*refCount);
 }
 }
 rCPtr& operator=(const rCPtr &right)
 {
 // Use copy and swap idiom
 rCPtr copy(right); // the minimum required to create object.
 swap(copy); // Swap current and new state.
 return *this;
 // The destructor on copy will do the correct thing in
 // decrementing the reference count and if required
 // deleting the object. 
 }
 rCPtr& operator=(const T* right)
 {
 // Use copy and swap idiom.
 rCPtr newRef(right);
 swap(newRef);
 return *this;
 }
 // Trivial operators.
 // kep these as one liners.
 T* operator->() const {return ptr; }
 T& operator*() const {return *ptr;}
 const T& get() const {return *ptr;}
 long getRefCount() const {return refCount ? *refCount : 0;}
 bool exists() const {return ptr != nullptr;} 
 explicit operator bool() const {return exists();}
 template <class X>
 X* dCast()
 {
 // X must be polymorphic, or else the cast will fail and cause a compiler error.
 return dynamic_cast<X*>(ptr);
 }
};

• Ragov: Thanks! If you don't mind in what cases would delete ptr fail? (other than if ptr was already deleted) Should I all the times where I delete ptr in a try/except block? The reason I added a new T in the constructor was because in one of my earlier iterations, not having that caused double free or corruption (still don't know why really) but, it doesn't now
• Loki Astari: @Ragov destructors can still throw exceptions. Though since C++11 you need to do extra work to make that happen. But because T is some random class that you have no control over you do need to take it into account.
• Ragov: I see! so the class itself can have a bug in it which causes it's construction to fail! Thanks! That also means since long is a built in type the only real way for it to fail is a bad alloc And by reading the conversation on the other answer, if there is a bad alloc, things are going down pretty soon.
• Loki Astari: @Ragov The thing about bad alloc being thrown is that it starts the stack being unwound so it can free up a lot of memory with all the objects being destroyed. So there are lots of situations were an exception may be caused by lack of resources but the exception causes things to recover to the point where the application can continue. So you should not assume that running out of memory means that things have failed.
• Ragov: Thanks (more things to study...) If I call delete does the pointer become null or just indeterminate (and if it is the second would having exists check refCount instead be better form. I do suppose setting it to nullptr could let it be quietly deleted multiple times though...