custom smart pointer class template
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
7
down vote
favorite
Kindly provide your review comments:
//SmartPtr.h
class RefCount
public:
void AddRef()
++(this->_count);
int Release()
return --(this->_count);
private:
int _count;
;
template <class T>
class SmartPtr
public:
// constructor
SmartPtr();
SmartPtr(T* iObject);
// copy constructor
SmartPtr(const SmartPtr<T>& iSPtr);
// destructor
~SmartPtr();
// operators
SmartPtr<T>& operator=(const SmartPtr<T>& iSPtr);
T& operator*();
T* operator->();
private:
T* _ptr;
RefCount* _refCount;
void _release();
void _copySmartPtr(const SmartPtr<T>& iSPtr);
;
//SmartPtr.cpp
#include "SmartPtr.h"
// constructor
template <class T>
SmartPtr<T>::SmartPtr():
_refCount(nullptr),
_ptr(nullptr)
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()),
_ptr(iObject)
this->_refCount->AddRef();
// copy constructor
template <class T>
SmartPtr<T>::SmartPtr(const SmartPtr<T>& iSPtr)
this->_copySmartPtr(iSPtr);
// destructor
template <class T>
SmartPtr<T>::~SmartPtr()
this->_release();
// operators
template <class T>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& iSPtr)
if (iSPtr._ptr && (this != &iSPtr))
this->_release();
this->_copySmartPtr(iSPtr);
return *this;
template <class T>
T& SmartPtr<T>::operator*()
return *(this->_ptr);
template <class T>
T* SmartPtr<T>::operator->()
return this->_ptr;
template <class T>
void SmartPtr<T>::_release()
if (this->_refCount && this->_refCount->Release() == 0)
delete this->_ptr;
delete this->_refCount;
template <class T>
void SmartPtr<T>::_copySmartPtr(const SmartPtr<T>& iSPtr)
this->_ptr = iSPtr._ptr;
this->_refCount = iSPtr._refCount;
this->_refCount->AddRef();
c++ pointers c++98
 |Â
show 8 more comments
up vote
7
down vote
favorite
Kindly provide your review comments:
//SmartPtr.h
class RefCount
public:
void AddRef()
++(this->_count);
int Release()
return --(this->_count);
private:
int _count;
;
template <class T>
class SmartPtr
public:
// constructor
SmartPtr();
SmartPtr(T* iObject);
// copy constructor
SmartPtr(const SmartPtr<T>& iSPtr);
// destructor
~SmartPtr();
// operators
SmartPtr<T>& operator=(const SmartPtr<T>& iSPtr);
T& operator*();
T* operator->();
private:
T* _ptr;
RefCount* _refCount;
void _release();
void _copySmartPtr(const SmartPtr<T>& iSPtr);
;
//SmartPtr.cpp
#include "SmartPtr.h"
// constructor
template <class T>
SmartPtr<T>::SmartPtr():
_refCount(nullptr),
_ptr(nullptr)
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()),
_ptr(iObject)
this->_refCount->AddRef();
// copy constructor
template <class T>
SmartPtr<T>::SmartPtr(const SmartPtr<T>& iSPtr)
this->_copySmartPtr(iSPtr);
// destructor
template <class T>
SmartPtr<T>::~SmartPtr()
this->_release();
// operators
template <class T>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& iSPtr)
if (iSPtr._ptr && (this != &iSPtr))
this->_release();
this->_copySmartPtr(iSPtr);
return *this;
template <class T>
T& SmartPtr<T>::operator*()
return *(this->_ptr);
template <class T>
T* SmartPtr<T>::operator->()
return this->_ptr;
template <class T>
void SmartPtr<T>::_release()
if (this->_refCount && this->_refCount->Release() == 0)
delete this->_ptr;
delete this->_refCount;
template <class T>
void SmartPtr<T>::_copySmartPtr(const SmartPtr<T>& iSPtr)
this->_ptr = iSPtr._ptr;
this->_refCount = iSPtr._refCount;
this->_refCount->AddRef();
c++ pointers c++98
5
What C++ version did you use? Is there a reason you wrote this instead of usingstd::unique_ptr
orstd::shared_ptr
?
â Mast
Aug 1 at 11:19
1
C++98 is still being taught? Both unique and shared have been added in C++11, a specification that came out 7 years ago and is supported by just about any compiler I know.
â Mast
Aug 1 at 13:08
4
Thanks Mast, Snowhawk. I appreciate your comments. I was just trying to get my hands on how to implement SmartPtr without using the shared_ptr/unique_ptr. I agree with you that it is better to use the c++11 features.
â Amit
Aug 1 at 13:40
1
@Amit: Reimplementing standard library smart pointers is one thing (and a fine goal on its own), but is it necessary to go back to C++98 to do so? C++11 is widely available, and if it isn't, lack of smart pointers likely isn't the biggest issue. (Just my impression, YMMV)
â hoffmale
Aug 1 at 14:29
4
This doesnâÂÂt merit its own answer but hereâÂÂs a mini-review: âÂÂSmartPtr
â is a bad name. It doesnâÂÂt tell the user what it actually does. Choose a name thatâÂÂs descriptive, such asSharedPtr
orRefCountedPtr
.
â Konrad Rudolph
Aug 1 at 16:36
 |Â
show 8 more comments
up vote
7
down vote
favorite
up vote
7
down vote
favorite
Kindly provide your review comments:
//SmartPtr.h
class RefCount
public:
void AddRef()
++(this->_count);
int Release()
return --(this->_count);
private:
int _count;
;
template <class T>
class SmartPtr
public:
// constructor
SmartPtr();
SmartPtr(T* iObject);
// copy constructor
SmartPtr(const SmartPtr<T>& iSPtr);
// destructor
~SmartPtr();
// operators
SmartPtr<T>& operator=(const SmartPtr<T>& iSPtr);
T& operator*();
T* operator->();
private:
T* _ptr;
RefCount* _refCount;
void _release();
void _copySmartPtr(const SmartPtr<T>& iSPtr);
;
//SmartPtr.cpp
#include "SmartPtr.h"
// constructor
template <class T>
SmartPtr<T>::SmartPtr():
_refCount(nullptr),
_ptr(nullptr)
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()),
_ptr(iObject)
this->_refCount->AddRef();
// copy constructor
template <class T>
SmartPtr<T>::SmartPtr(const SmartPtr<T>& iSPtr)
this->_copySmartPtr(iSPtr);
// destructor
template <class T>
SmartPtr<T>::~SmartPtr()
this->_release();
// operators
template <class T>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& iSPtr)
if (iSPtr._ptr && (this != &iSPtr))
this->_release();
this->_copySmartPtr(iSPtr);
return *this;
template <class T>
T& SmartPtr<T>::operator*()
return *(this->_ptr);
template <class T>
T* SmartPtr<T>::operator->()
return this->_ptr;
template <class T>
void SmartPtr<T>::_release()
if (this->_refCount && this->_refCount->Release() == 0)
delete this->_ptr;
delete this->_refCount;
template <class T>
void SmartPtr<T>::_copySmartPtr(const SmartPtr<T>& iSPtr)
this->_ptr = iSPtr._ptr;
this->_refCount = iSPtr._refCount;
this->_refCount->AddRef();
c++ pointers c++98
Kindly provide your review comments:
//SmartPtr.h
class RefCount
public:
void AddRef()
++(this->_count);
int Release()
return --(this->_count);
private:
int _count;
;
template <class T>
class SmartPtr
public:
// constructor
SmartPtr();
SmartPtr(T* iObject);
// copy constructor
SmartPtr(const SmartPtr<T>& iSPtr);
// destructor
~SmartPtr();
// operators
SmartPtr<T>& operator=(const SmartPtr<T>& iSPtr);
T& operator*();
T* operator->();
private:
T* _ptr;
RefCount* _refCount;
void _release();
void _copySmartPtr(const SmartPtr<T>& iSPtr);
;
//SmartPtr.cpp
#include "SmartPtr.h"
// constructor
template <class T>
SmartPtr<T>::SmartPtr():
_refCount(nullptr),
_ptr(nullptr)
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()),
_ptr(iObject)
this->_refCount->AddRef();
// copy constructor
template <class T>
SmartPtr<T>::SmartPtr(const SmartPtr<T>& iSPtr)
this->_copySmartPtr(iSPtr);
// destructor
template <class T>
SmartPtr<T>::~SmartPtr()
this->_release();
// operators
template <class T>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& iSPtr)
if (iSPtr._ptr && (this != &iSPtr))
this->_release();
this->_copySmartPtr(iSPtr);
return *this;
template <class T>
T& SmartPtr<T>::operator*()
return *(this->_ptr);
template <class T>
T* SmartPtr<T>::operator->()
return this->_ptr;
template <class T>
void SmartPtr<T>::_release()
if (this->_refCount && this->_refCount->Release() == 0)
delete this->_ptr;
delete this->_refCount;
template <class T>
void SmartPtr<T>::_copySmartPtr(const SmartPtr<T>& iSPtr)
this->_ptr = iSPtr._ptr;
this->_refCount = iSPtr._refCount;
this->_refCount->AddRef();
c++ pointers c++98
edited Aug 1 at 11:57
Toby Speight
17.1k13485
17.1k13485
asked Aug 1 at 10:54
Amit
385
385
5
What C++ version did you use? Is there a reason you wrote this instead of usingstd::unique_ptr
orstd::shared_ptr
?
â Mast
Aug 1 at 11:19
1
C++98 is still being taught? Both unique and shared have been added in C++11, a specification that came out 7 years ago and is supported by just about any compiler I know.
â Mast
Aug 1 at 13:08
4
Thanks Mast, Snowhawk. I appreciate your comments. I was just trying to get my hands on how to implement SmartPtr without using the shared_ptr/unique_ptr. I agree with you that it is better to use the c++11 features.
â Amit
Aug 1 at 13:40
1
@Amit: Reimplementing standard library smart pointers is one thing (and a fine goal on its own), but is it necessary to go back to C++98 to do so? C++11 is widely available, and if it isn't, lack of smart pointers likely isn't the biggest issue. (Just my impression, YMMV)
â hoffmale
Aug 1 at 14:29
4
This doesnâÂÂt merit its own answer but hereâÂÂs a mini-review: âÂÂSmartPtr
â is a bad name. It doesnâÂÂt tell the user what it actually does. Choose a name thatâÂÂs descriptive, such asSharedPtr
orRefCountedPtr
.
â Konrad Rudolph
Aug 1 at 16:36
 |Â
show 8 more comments
5
What C++ version did you use? Is there a reason you wrote this instead of usingstd::unique_ptr
orstd::shared_ptr
?
â Mast
Aug 1 at 11:19
1
C++98 is still being taught? Both unique and shared have been added in C++11, a specification that came out 7 years ago and is supported by just about any compiler I know.
â Mast
Aug 1 at 13:08
4
Thanks Mast, Snowhawk. I appreciate your comments. I was just trying to get my hands on how to implement SmartPtr without using the shared_ptr/unique_ptr. I agree with you that it is better to use the c++11 features.
â Amit
Aug 1 at 13:40
1
@Amit: Reimplementing standard library smart pointers is one thing (and a fine goal on its own), but is it necessary to go back to C++98 to do so? C++11 is widely available, and if it isn't, lack of smart pointers likely isn't the biggest issue. (Just my impression, YMMV)
â hoffmale
Aug 1 at 14:29
4
This doesnâÂÂt merit its own answer but hereâÂÂs a mini-review: âÂÂSmartPtr
â is a bad name. It doesnâÂÂt tell the user what it actually does. Choose a name thatâÂÂs descriptive, such asSharedPtr
orRefCountedPtr
.
â Konrad Rudolph
Aug 1 at 16:36
5
5
What C++ version did you use? Is there a reason you wrote this instead of using
std::unique_ptr
or std::shared_ptr
?â Mast
Aug 1 at 11:19
What C++ version did you use? Is there a reason you wrote this instead of using
std::unique_ptr
or std::shared_ptr
?â Mast
Aug 1 at 11:19
1
1
C++98 is still being taught? Both unique and shared have been added in C++11, a specification that came out 7 years ago and is supported by just about any compiler I know.
â Mast
Aug 1 at 13:08
C++98 is still being taught? Both unique and shared have been added in C++11, a specification that came out 7 years ago and is supported by just about any compiler I know.
â Mast
Aug 1 at 13:08
4
4
Thanks Mast, Snowhawk. I appreciate your comments. I was just trying to get my hands on how to implement SmartPtr without using the shared_ptr/unique_ptr. I agree with you that it is better to use the c++11 features.
â Amit
Aug 1 at 13:40
Thanks Mast, Snowhawk. I appreciate your comments. I was just trying to get my hands on how to implement SmartPtr without using the shared_ptr/unique_ptr. I agree with you that it is better to use the c++11 features.
â Amit
Aug 1 at 13:40
1
1
@Amit: Reimplementing standard library smart pointers is one thing (and a fine goal on its own), but is it necessary to go back to C++98 to do so? C++11 is widely available, and if it isn't, lack of smart pointers likely isn't the biggest issue. (Just my impression, YMMV)
â hoffmale
Aug 1 at 14:29
@Amit: Reimplementing standard library smart pointers is one thing (and a fine goal on its own), but is it necessary to go back to C++98 to do so? C++11 is widely available, and if it isn't, lack of smart pointers likely isn't the biggest issue. (Just my impression, YMMV)
â hoffmale
Aug 1 at 14:29
4
4
This doesnâÂÂt merit its own answer but hereâÂÂs a mini-review: âÂÂ
SmartPtr
â is a bad name. It doesnâÂÂt tell the user what it actually does. Choose a name thatâÂÂs descriptive, such as SharedPtr
or RefCountedPtr
.â Konrad Rudolph
Aug 1 at 16:36
This doesnâÂÂt merit its own answer but hereâÂÂs a mini-review: âÂÂ
SmartPtr
â is a bad name. It doesnâÂÂt tell the user what it actually does. Choose a name thatâÂÂs descriptive, such as SharedPtr
or RefCountedPtr
.â Konrad Rudolph
Aug 1 at 16:36
 |Â
show 8 more comments
4 Answers
4
active
oldest
votes
up vote
7
down vote
accepted
The Big Issue
One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()), // This can throw.
_ptr(iObject)
this->_refCount->AddRef();
If the creation of the _refCount
object fails you will leak the pointer. You need to guarantee that the pointer does not leak:
SmartPtr<T>::SmartPtr(T* iObject)
try // Add a try block
:_refCount(new RefCount())
,_ptr(iObject)
this->_refCount->AddRef();
catch(...)
delete iObject; // If there is an exception make sure the object is deleted.
throw; // Then re-throw the exception.
Minor Missing Functions
The main missing things I see are:
T* get(); // Sometimes its nice to just get the pointer.
operator bool(); // Nice when you want to simply check the smart
// pointer has something in a boolean context.
SmartPtr<Stuff> data = getSmartPointer();
if (data)
data->doWork(); // Need to check there is a pointer in the
// object before calling doWork().
Constructor Problems
Explicit
Your one parameter constructor should be explicit
. Think of this situation.
void doStuff(SmartPtr<Stuff> work)
if (work)
work->doWork();
Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.
But what happens when I do this:
int main()
Stuff* work = new Stuff;
doStuff(work);
work->moreActions();
delete work;
This code compiles. But the call to doStuff()
results in delete being called on the work
object. Even though you though it was safe to call (as you are making a copy of smart pointer).
The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.
What About nullptr
Your object does not accept a nullptr!
SmartPtr<Stuff> value = nullptr; // fails to compile.
It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.
What About Derived Types.
On of the major things about C++ is derived types with more functionality.
Derived* x = new Derived;
Base* y = x;
The same should work with smart pointers.
SmartPtr<Derived> x = new Derived;
SmartPtr<Base> y = x; // fails to compile.
This will be a common in most C++ code (not exactly like this). But this functionality is really needed.
Make Shared
One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.
They remedied this by introducing std::make_shared<T>()
. This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.
Further Reading
I cover a lot of these details and more in some articles I wrote:
Series
Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors
nullptr
didn't exist in C++98.operator bool()
should use the old safe-bool idiom.
â Snowhawk
Aug 1 at 17:08
1
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
Won't the blatantdelete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.
â rubenvb
Aug 2 at 9:28
@rubenvb TheiObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.
â Martin York
Aug 2 at 15:44
add a comment |Â
up vote
7
down vote
As @bipll mentioned,
template
s must be fully available everywhere they are used. Maybe move the implementation to the header, or at least add a note that the .cpp file is intended to be included instead of the header, as this is unusual.Some operators (
*
,->
) needconst
overloads (or need to be madeconst
), as otherwise the contained pointer isn't be accessed from aconst
context, e.g. aconst SmartPtr<T>&
.Please provide an easier access to the underlying pointer than
smart_ptr.operator->()
. There are functions that only accept raw pointers, for which this way of accessingSmartPtr<T>::_ptr
gets cumbersome.This implementation seems similar to
std::shared_ptr
in the sense that theSmartPtr
is intended to be shared. However, if this "being shared" involves multiple threads, then accesses toRefCount::_count
need to be synchronized. (Otherwise you get race conditions.)
Sadly, C++98 doesn't offer any help for this in the standard library (as the notion of threads only got formally introduced to C++11), so you have to rely on some external library and/or platform dependent operations.
nullptr
only got introduced in C++11, so a C++98 compiler shouldn't be able to compile this.
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
add a comment |Â
up vote
7
down vote
There's no need to use
this->
to access any of the members of this class. Simply refer to them in the natural way:return --_count;
// not: return --(this->_count);The
RefCount
class doesn't need to be visible outside of the pointer implementation, so it can be a private class withinSmartPtr
.The code has severe problems with concurrency - however, that's hard to deal with portably, before the current memory model and introduction of
std::atomic
types.
add a comment |Â
up vote
5
down vote
Consider the null case a bit more
If the smart pointer hasn't been assigned, then you have a null pointer by default. That seems fine, but if I call *sptr
on an instance, I will get a null reference, which is UB.
This code, then, relies on the caller never calling operator*
when the smart pointer is in this (evidently acceptable) state. To check that, the caller will have to write something like:
if(sptr())
doThingWithReference(*sptr);
I would be inclined to make 2 changes, then: throw if the dereference operator is called when the pointer is null (exceptions are better than UB), and also to define a bool operator:
explicit operator bool() const
return _ptr == nullptr;
You mention C++98 in a comment; this explicit
version is only C++11 (even if you swapped nullptr for 0), prior to that you had to do interesting things to prevent a bool operator making way for int casting and so on.
Assignment to a null smart pointer will add references to nothing
Consider SmartPtr<A> sptr_a = sptr_null
: you Release a reference in the sptr_a refcount, then assign the pointer to that in sptr_null
(which is a null pointer), and then add 1 to the mutually held nothing.
If you then assign this again to a different smart pointer sptr_a = sptr_b
, then the assignment operator will refuse to _release the old pointer (pointing to nothing), and refuse to copy in the new one, too. If you have received a SmartPtr as a return value and assigned it to such a pointer like this:
SmartPtr<A> sptr_a = sptr_null;
sptr_a = GetJewel();
Then sptr_a
will still have a null pointer and the jewel will be thrown away.
Initialise your reference count
At present, there is nothing to initialise the reference count itself to zero except some good luck. The compiler should have warned you about that.
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a checkif(_refCount) _refCount->AddRef();
instead of creating a special casesptr_null
value?
â hoffmale
Aug 1 at 17:52
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
add a comment |Â
4 Answers
4
active
oldest
votes
4 Answers
4
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
7
down vote
accepted
The Big Issue
One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()), // This can throw.
_ptr(iObject)
this->_refCount->AddRef();
If the creation of the _refCount
object fails you will leak the pointer. You need to guarantee that the pointer does not leak:
SmartPtr<T>::SmartPtr(T* iObject)
try // Add a try block
:_refCount(new RefCount())
,_ptr(iObject)
this->_refCount->AddRef();
catch(...)
delete iObject; // If there is an exception make sure the object is deleted.
throw; // Then re-throw the exception.
Minor Missing Functions
The main missing things I see are:
T* get(); // Sometimes its nice to just get the pointer.
operator bool(); // Nice when you want to simply check the smart
// pointer has something in a boolean context.
SmartPtr<Stuff> data = getSmartPointer();
if (data)
data->doWork(); // Need to check there is a pointer in the
// object before calling doWork().
Constructor Problems
Explicit
Your one parameter constructor should be explicit
. Think of this situation.
void doStuff(SmartPtr<Stuff> work)
if (work)
work->doWork();
Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.
But what happens when I do this:
int main()
Stuff* work = new Stuff;
doStuff(work);
work->moreActions();
delete work;
This code compiles. But the call to doStuff()
results in delete being called on the work
object. Even though you though it was safe to call (as you are making a copy of smart pointer).
The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.
What About nullptr
Your object does not accept a nullptr!
SmartPtr<Stuff> value = nullptr; // fails to compile.
It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.
What About Derived Types.
On of the major things about C++ is derived types with more functionality.
Derived* x = new Derived;
Base* y = x;
The same should work with smart pointers.
SmartPtr<Derived> x = new Derived;
SmartPtr<Base> y = x; // fails to compile.
This will be a common in most C++ code (not exactly like this). But this functionality is really needed.
Make Shared
One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.
They remedied this by introducing std::make_shared<T>()
. This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.
Further Reading
I cover a lot of these details and more in some articles I wrote:
Series
Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors
nullptr
didn't exist in C++98.operator bool()
should use the old safe-bool idiom.
â Snowhawk
Aug 1 at 17:08
1
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
Won't the blatantdelete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.
â rubenvb
Aug 2 at 9:28
@rubenvb TheiObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.
â Martin York
Aug 2 at 15:44
add a comment |Â
up vote
7
down vote
accepted
The Big Issue
One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()), // This can throw.
_ptr(iObject)
this->_refCount->AddRef();
If the creation of the _refCount
object fails you will leak the pointer. You need to guarantee that the pointer does not leak:
SmartPtr<T>::SmartPtr(T* iObject)
try // Add a try block
:_refCount(new RefCount())
,_ptr(iObject)
this->_refCount->AddRef();
catch(...)
delete iObject; // If there is an exception make sure the object is deleted.
throw; // Then re-throw the exception.
Minor Missing Functions
The main missing things I see are:
T* get(); // Sometimes its nice to just get the pointer.
operator bool(); // Nice when you want to simply check the smart
// pointer has something in a boolean context.
SmartPtr<Stuff> data = getSmartPointer();
if (data)
data->doWork(); // Need to check there is a pointer in the
// object before calling doWork().
Constructor Problems
Explicit
Your one parameter constructor should be explicit
. Think of this situation.
void doStuff(SmartPtr<Stuff> work)
if (work)
work->doWork();
Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.
But what happens when I do this:
int main()
Stuff* work = new Stuff;
doStuff(work);
work->moreActions();
delete work;
This code compiles. But the call to doStuff()
results in delete being called on the work
object. Even though you though it was safe to call (as you are making a copy of smart pointer).
The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.
What About nullptr
Your object does not accept a nullptr!
SmartPtr<Stuff> value = nullptr; // fails to compile.
It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.
What About Derived Types.
On of the major things about C++ is derived types with more functionality.
Derived* x = new Derived;
Base* y = x;
The same should work with smart pointers.
SmartPtr<Derived> x = new Derived;
SmartPtr<Base> y = x; // fails to compile.
This will be a common in most C++ code (not exactly like this). But this functionality is really needed.
Make Shared
One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.
They remedied this by introducing std::make_shared<T>()
. This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.
Further Reading
I cover a lot of these details and more in some articles I wrote:
Series
Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors
nullptr
didn't exist in C++98.operator bool()
should use the old safe-bool idiom.
â Snowhawk
Aug 1 at 17:08
1
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
Won't the blatantdelete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.
â rubenvb
Aug 2 at 9:28
@rubenvb TheiObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.
â Martin York
Aug 2 at 15:44
add a comment |Â
up vote
7
down vote
accepted
up vote
7
down vote
accepted
The Big Issue
One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()), // This can throw.
_ptr(iObject)
this->_refCount->AddRef();
If the creation of the _refCount
object fails you will leak the pointer. You need to guarantee that the pointer does not leak:
SmartPtr<T>::SmartPtr(T* iObject)
try // Add a try block
:_refCount(new RefCount())
,_ptr(iObject)
this->_refCount->AddRef();
catch(...)
delete iObject; // If there is an exception make sure the object is deleted.
throw; // Then re-throw the exception.
Minor Missing Functions
The main missing things I see are:
T* get(); // Sometimes its nice to just get the pointer.
operator bool(); // Nice when you want to simply check the smart
// pointer has something in a boolean context.
SmartPtr<Stuff> data = getSmartPointer();
if (data)
data->doWork(); // Need to check there is a pointer in the
// object before calling doWork().
Constructor Problems
Explicit
Your one parameter constructor should be explicit
. Think of this situation.
void doStuff(SmartPtr<Stuff> work)
if (work)
work->doWork();
Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.
But what happens when I do this:
int main()
Stuff* work = new Stuff;
doStuff(work);
work->moreActions();
delete work;
This code compiles. But the call to doStuff()
results in delete being called on the work
object. Even though you though it was safe to call (as you are making a copy of smart pointer).
The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.
What About nullptr
Your object does not accept a nullptr!
SmartPtr<Stuff> value = nullptr; // fails to compile.
It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.
What About Derived Types.
On of the major things about C++ is derived types with more functionality.
Derived* x = new Derived;
Base* y = x;
The same should work with smart pointers.
SmartPtr<Derived> x = new Derived;
SmartPtr<Base> y = x; // fails to compile.
This will be a common in most C++ code (not exactly like this). But this functionality is really needed.
Make Shared
One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.
They remedied this by introducing std::make_shared<T>()
. This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.
Further Reading
I cover a lot of these details and more in some articles I wrote:
Series
Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors
The Big Issue
One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.
template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
_refCount(new RefCount()), // This can throw.
_ptr(iObject)
this->_refCount->AddRef();
If the creation of the _refCount
object fails you will leak the pointer. You need to guarantee that the pointer does not leak:
SmartPtr<T>::SmartPtr(T* iObject)
try // Add a try block
:_refCount(new RefCount())
,_ptr(iObject)
this->_refCount->AddRef();
catch(...)
delete iObject; // If there is an exception make sure the object is deleted.
throw; // Then re-throw the exception.
Minor Missing Functions
The main missing things I see are:
T* get(); // Sometimes its nice to just get the pointer.
operator bool(); // Nice when you want to simply check the smart
// pointer has something in a boolean context.
SmartPtr<Stuff> data = getSmartPointer();
if (data)
data->doWork(); // Need to check there is a pointer in the
// object before calling doWork().
Constructor Problems
Explicit
Your one parameter constructor should be explicit
. Think of this situation.
void doStuff(SmartPtr<Stuff> work)
if (work)
work->doWork();
Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.
But what happens when I do this:
int main()
Stuff* work = new Stuff;
doStuff(work);
work->moreActions();
delete work;
This code compiles. But the call to doStuff()
results in delete being called on the work
object. Even though you though it was safe to call (as you are making a copy of smart pointer).
The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.
What About nullptr
Your object does not accept a nullptr!
SmartPtr<Stuff> value = nullptr; // fails to compile.
It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.
What About Derived Types.
On of the major things about C++ is derived types with more functionality.
Derived* x = new Derived;
Base* y = x;
The same should work with smart pointers.
SmartPtr<Derived> x = new Derived;
SmartPtr<Base> y = x; // fails to compile.
This will be a common in most C++ code (not exactly like this). But this functionality is really needed.
Make Shared
One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.
They remedied this by introducing std::make_shared<T>()
. This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.
Further Reading
I cover a lot of these details and more in some articles I wrote:
Series
Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors
answered Aug 1 at 16:32
Martin York
70.7k481244
70.7k481244
nullptr
didn't exist in C++98.operator bool()
should use the old safe-bool idiom.
â Snowhawk
Aug 1 at 17:08
1
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
Won't the blatantdelete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.
â rubenvb
Aug 2 at 9:28
@rubenvb TheiObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.
â Martin York
Aug 2 at 15:44
add a comment |Â
nullptr
didn't exist in C++98.operator bool()
should use the old safe-bool idiom.
â Snowhawk
Aug 1 at 17:08
1
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
Won't the blatantdelete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.
â rubenvb
Aug 2 at 9:28
@rubenvb TheiObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.
â Martin York
Aug 2 at 15:44
nullptr
didn't exist in C++98. operator bool()
should use the old safe-bool idiom.â Snowhawk
Aug 1 at 17:08
nullptr
didn't exist in C++98. operator bool()
should use the old safe-bool idiom.â Snowhawk
Aug 1 at 17:08
1
1
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
@Snowhawk Must admit I missed the C++98 mark. But that version is over 2 decades old.
â Martin York
Aug 1 at 18:35
Won't the blatant
delete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.â rubenvb
Aug 2 at 9:28
Won't the blatant
delete iObject
possible try to delete a random memory address? If the constructor throws, the member is never initialised. Calling delete on an uninitialized pointer is undefined behaviour.â rubenvb
Aug 2 at 9:28
@rubenvb The
iObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.â Martin York
Aug 2 at 15:44
@rubenvb The
iObject
is passed by the caller. The contract with the caller is that the class WILL call delete on this object (even if the constructor throws). Otherwise you are left in a position where the smart pointer may leak.â Martin York
Aug 2 at 15:44
add a comment |Â
up vote
7
down vote
As @bipll mentioned,
template
s must be fully available everywhere they are used. Maybe move the implementation to the header, or at least add a note that the .cpp file is intended to be included instead of the header, as this is unusual.Some operators (
*
,->
) needconst
overloads (or need to be madeconst
), as otherwise the contained pointer isn't be accessed from aconst
context, e.g. aconst SmartPtr<T>&
.Please provide an easier access to the underlying pointer than
smart_ptr.operator->()
. There are functions that only accept raw pointers, for which this way of accessingSmartPtr<T>::_ptr
gets cumbersome.This implementation seems similar to
std::shared_ptr
in the sense that theSmartPtr
is intended to be shared. However, if this "being shared" involves multiple threads, then accesses toRefCount::_count
need to be synchronized. (Otherwise you get race conditions.)
Sadly, C++98 doesn't offer any help for this in the standard library (as the notion of threads only got formally introduced to C++11), so you have to rely on some external library and/or platform dependent operations.
nullptr
only got introduced in C++11, so a C++98 compiler shouldn't be able to compile this.
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
add a comment |Â
up vote
7
down vote
As @bipll mentioned,
template
s must be fully available everywhere they are used. Maybe move the implementation to the header, or at least add a note that the .cpp file is intended to be included instead of the header, as this is unusual.Some operators (
*
,->
) needconst
overloads (or need to be madeconst
), as otherwise the contained pointer isn't be accessed from aconst
context, e.g. aconst SmartPtr<T>&
.Please provide an easier access to the underlying pointer than
smart_ptr.operator->()
. There are functions that only accept raw pointers, for which this way of accessingSmartPtr<T>::_ptr
gets cumbersome.This implementation seems similar to
std::shared_ptr
in the sense that theSmartPtr
is intended to be shared. However, if this "being shared" involves multiple threads, then accesses toRefCount::_count
need to be synchronized. (Otherwise you get race conditions.)
Sadly, C++98 doesn't offer any help for this in the standard library (as the notion of threads only got formally introduced to C++11), so you have to rely on some external library and/or platform dependent operations.
nullptr
only got introduced in C++11, so a C++98 compiler shouldn't be able to compile this.
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
add a comment |Â
up vote
7
down vote
up vote
7
down vote
As @bipll mentioned,
template
s must be fully available everywhere they are used. Maybe move the implementation to the header, or at least add a note that the .cpp file is intended to be included instead of the header, as this is unusual.Some operators (
*
,->
) needconst
overloads (or need to be madeconst
), as otherwise the contained pointer isn't be accessed from aconst
context, e.g. aconst SmartPtr<T>&
.Please provide an easier access to the underlying pointer than
smart_ptr.operator->()
. There are functions that only accept raw pointers, for which this way of accessingSmartPtr<T>::_ptr
gets cumbersome.This implementation seems similar to
std::shared_ptr
in the sense that theSmartPtr
is intended to be shared. However, if this "being shared" involves multiple threads, then accesses toRefCount::_count
need to be synchronized. (Otherwise you get race conditions.)
Sadly, C++98 doesn't offer any help for this in the standard library (as the notion of threads only got formally introduced to C++11), so you have to rely on some external library and/or platform dependent operations.
nullptr
only got introduced in C++11, so a C++98 compiler shouldn't be able to compile this.
As @bipll mentioned,
template
s must be fully available everywhere they are used. Maybe move the implementation to the header, or at least add a note that the .cpp file is intended to be included instead of the header, as this is unusual.Some operators (
*
,->
) needconst
overloads (or need to be madeconst
), as otherwise the contained pointer isn't be accessed from aconst
context, e.g. aconst SmartPtr<T>&
.Please provide an easier access to the underlying pointer than
smart_ptr.operator->()
. There are functions that only accept raw pointers, for which this way of accessingSmartPtr<T>::_ptr
gets cumbersome.This implementation seems similar to
std::shared_ptr
in the sense that theSmartPtr
is intended to be shared. However, if this "being shared" involves multiple threads, then accesses toRefCount::_count
need to be synchronized. (Otherwise you get race conditions.)
Sadly, C++98 doesn't offer any help for this in the standard library (as the notion of threads only got formally introduced to C++11), so you have to rely on some external library and/or platform dependent operations.
nullptr
only got introduced in C++11, so a C++98 compiler shouldn't be able to compile this.
answered Aug 1 at 11:57
hoffmale
4,195630
4,195630
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
add a comment |Â
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
ItâÂÂs not âÂÂjustâ unusual, it flat out breaks some build systems. DonâÂÂt do it.
â Konrad Rudolph
Aug 1 at 16:34
add a comment |Â
up vote
7
down vote
There's no need to use
this->
to access any of the members of this class. Simply refer to them in the natural way:return --_count;
// not: return --(this->_count);The
RefCount
class doesn't need to be visible outside of the pointer implementation, so it can be a private class withinSmartPtr
.The code has severe problems with concurrency - however, that's hard to deal with portably, before the current memory model and introduction of
std::atomic
types.
add a comment |Â
up vote
7
down vote
There's no need to use
this->
to access any of the members of this class. Simply refer to them in the natural way:return --_count;
// not: return --(this->_count);The
RefCount
class doesn't need to be visible outside of the pointer implementation, so it can be a private class withinSmartPtr
.The code has severe problems with concurrency - however, that's hard to deal with portably, before the current memory model and introduction of
std::atomic
types.
add a comment |Â
up vote
7
down vote
up vote
7
down vote
There's no need to use
this->
to access any of the members of this class. Simply refer to them in the natural way:return --_count;
// not: return --(this->_count);The
RefCount
class doesn't need to be visible outside of the pointer implementation, so it can be a private class withinSmartPtr
.The code has severe problems with concurrency - however, that's hard to deal with portably, before the current memory model and introduction of
std::atomic
types.
There's no need to use
this->
to access any of the members of this class. Simply refer to them in the natural way:return --_count;
// not: return --(this->_count);The
RefCount
class doesn't need to be visible outside of the pointer implementation, so it can be a private class withinSmartPtr
.The code has severe problems with concurrency - however, that's hard to deal with portably, before the current memory model and introduction of
std::atomic
types.
answered Aug 1 at 12:02
Toby Speight
17.1k13485
17.1k13485
add a comment |Â
add a comment |Â
up vote
5
down vote
Consider the null case a bit more
If the smart pointer hasn't been assigned, then you have a null pointer by default. That seems fine, but if I call *sptr
on an instance, I will get a null reference, which is UB.
This code, then, relies on the caller never calling operator*
when the smart pointer is in this (evidently acceptable) state. To check that, the caller will have to write something like:
if(sptr())
doThingWithReference(*sptr);
I would be inclined to make 2 changes, then: throw if the dereference operator is called when the pointer is null (exceptions are better than UB), and also to define a bool operator:
explicit operator bool() const
return _ptr == nullptr;
You mention C++98 in a comment; this explicit
version is only C++11 (even if you swapped nullptr for 0), prior to that you had to do interesting things to prevent a bool operator making way for int casting and so on.
Assignment to a null smart pointer will add references to nothing
Consider SmartPtr<A> sptr_a = sptr_null
: you Release a reference in the sptr_a refcount, then assign the pointer to that in sptr_null
(which is a null pointer), and then add 1 to the mutually held nothing.
If you then assign this again to a different smart pointer sptr_a = sptr_b
, then the assignment operator will refuse to _release the old pointer (pointing to nothing), and refuse to copy in the new one, too. If you have received a SmartPtr as a return value and assigned it to such a pointer like this:
SmartPtr<A> sptr_a = sptr_null;
sptr_a = GetJewel();
Then sptr_a
will still have a null pointer and the jewel will be thrown away.
Initialise your reference count
At present, there is nothing to initialise the reference count itself to zero except some good luck. The compiler should have warned you about that.
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a checkif(_refCount) _refCount->AddRef();
instead of creating a special casesptr_null
value?
â hoffmale
Aug 1 at 17:52
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
add a comment |Â
up vote
5
down vote
Consider the null case a bit more
If the smart pointer hasn't been assigned, then you have a null pointer by default. That seems fine, but if I call *sptr
on an instance, I will get a null reference, which is UB.
This code, then, relies on the caller never calling operator*
when the smart pointer is in this (evidently acceptable) state. To check that, the caller will have to write something like:
if(sptr())
doThingWithReference(*sptr);
I would be inclined to make 2 changes, then: throw if the dereference operator is called when the pointer is null (exceptions are better than UB), and also to define a bool operator:
explicit operator bool() const
return _ptr == nullptr;
You mention C++98 in a comment; this explicit
version is only C++11 (even if you swapped nullptr for 0), prior to that you had to do interesting things to prevent a bool operator making way for int casting and so on.
Assignment to a null smart pointer will add references to nothing
Consider SmartPtr<A> sptr_a = sptr_null
: you Release a reference in the sptr_a refcount, then assign the pointer to that in sptr_null
(which is a null pointer), and then add 1 to the mutually held nothing.
If you then assign this again to a different smart pointer sptr_a = sptr_b
, then the assignment operator will refuse to _release the old pointer (pointing to nothing), and refuse to copy in the new one, too. If you have received a SmartPtr as a return value and assigned it to such a pointer like this:
SmartPtr<A> sptr_a = sptr_null;
sptr_a = GetJewel();
Then sptr_a
will still have a null pointer and the jewel will be thrown away.
Initialise your reference count
At present, there is nothing to initialise the reference count itself to zero except some good luck. The compiler should have warned you about that.
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a checkif(_refCount) _refCount->AddRef();
instead of creating a special casesptr_null
value?
â hoffmale
Aug 1 at 17:52
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
add a comment |Â
up vote
5
down vote
up vote
5
down vote
Consider the null case a bit more
If the smart pointer hasn't been assigned, then you have a null pointer by default. That seems fine, but if I call *sptr
on an instance, I will get a null reference, which is UB.
This code, then, relies on the caller never calling operator*
when the smart pointer is in this (evidently acceptable) state. To check that, the caller will have to write something like:
if(sptr())
doThingWithReference(*sptr);
I would be inclined to make 2 changes, then: throw if the dereference operator is called when the pointer is null (exceptions are better than UB), and also to define a bool operator:
explicit operator bool() const
return _ptr == nullptr;
You mention C++98 in a comment; this explicit
version is only C++11 (even if you swapped nullptr for 0), prior to that you had to do interesting things to prevent a bool operator making way for int casting and so on.
Assignment to a null smart pointer will add references to nothing
Consider SmartPtr<A> sptr_a = sptr_null
: you Release a reference in the sptr_a refcount, then assign the pointer to that in sptr_null
(which is a null pointer), and then add 1 to the mutually held nothing.
If you then assign this again to a different smart pointer sptr_a = sptr_b
, then the assignment operator will refuse to _release the old pointer (pointing to nothing), and refuse to copy in the new one, too. If you have received a SmartPtr as a return value and assigned it to such a pointer like this:
SmartPtr<A> sptr_a = sptr_null;
sptr_a = GetJewel();
Then sptr_a
will still have a null pointer and the jewel will be thrown away.
Initialise your reference count
At present, there is nothing to initialise the reference count itself to zero except some good luck. The compiler should have warned you about that.
Consider the null case a bit more
If the smart pointer hasn't been assigned, then you have a null pointer by default. That seems fine, but if I call *sptr
on an instance, I will get a null reference, which is UB.
This code, then, relies on the caller never calling operator*
when the smart pointer is in this (evidently acceptable) state. To check that, the caller will have to write something like:
if(sptr())
doThingWithReference(*sptr);
I would be inclined to make 2 changes, then: throw if the dereference operator is called when the pointer is null (exceptions are better than UB), and also to define a bool operator:
explicit operator bool() const
return _ptr == nullptr;
You mention C++98 in a comment; this explicit
version is only C++11 (even if you swapped nullptr for 0), prior to that you had to do interesting things to prevent a bool operator making way for int casting and so on.
Assignment to a null smart pointer will add references to nothing
Consider SmartPtr<A> sptr_a = sptr_null
: you Release a reference in the sptr_a refcount, then assign the pointer to that in sptr_null
(which is a null pointer), and then add 1 to the mutually held nothing.
If you then assign this again to a different smart pointer sptr_a = sptr_b
, then the assignment operator will refuse to _release the old pointer (pointing to nothing), and refuse to copy in the new one, too. If you have received a SmartPtr as a return value and assigned it to such a pointer like this:
SmartPtr<A> sptr_a = sptr_null;
sptr_a = GetJewel();
Then sptr_a
will still have a null pointer and the jewel will be thrown away.
Initialise your reference count
At present, there is nothing to initialise the reference count itself to zero except some good luck. The compiler should have warned you about that.
answered Aug 1 at 14:05
Phil H
38647
38647
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a checkif(_refCount) _refCount->AddRef();
instead of creating a special casesptr_null
value?
â hoffmale
Aug 1 at 17:52
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
add a comment |Â
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a checkif(_refCount) _refCount->AddRef();
instead of creating a special casesptr_null
value?
â hoffmale
Aug 1 at 17:52
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a check
if(_refCount) _refCount->AddRef();
instead of creating a special case sptr_null
value?â hoffmale
Aug 1 at 17:52
I agree with section #1 and #3 (though value initialization takes care of that since C++03), but section #2 seems weird: Why do we need to track the number of refences to "nothing"? Wouldn't it be easier to simply add a check
if(_refCount) _refCount->AddRef();
instead of creating a special case sptr_null
value?â hoffmale
Aug 1 at 17:52
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
@hoffmale: Sorry if that bit is unclear, it's not meant to be a special case, it's just meant to be an instance of the class which is pointing to null, and I'm illustrating a case where the bug shows itself.
â Phil H
Aug 1 at 20:56
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f200737%2fcustom-smart-pointer-class-template%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
5
What C++ version did you use? Is there a reason you wrote this instead of using
std::unique_ptr
orstd::shared_ptr
?â Mast
Aug 1 at 11:19
1
C++98 is still being taught? Both unique and shared have been added in C++11, a specification that came out 7 years ago and is supported by just about any compiler I know.
â Mast
Aug 1 at 13:08
4
Thanks Mast, Snowhawk. I appreciate your comments. I was just trying to get my hands on how to implement SmartPtr without using the shared_ptr/unique_ptr. I agree with you that it is better to use the c++11 features.
â Amit
Aug 1 at 13:40
1
@Amit: Reimplementing standard library smart pointers is one thing (and a fine goal on its own), but is it necessary to go back to C++98 to do so? C++11 is widely available, and if it isn't, lack of smart pointers likely isn't the biggest issue. (Just my impression, YMMV)
â hoffmale
Aug 1 at 14:29
4
This doesnâÂÂt merit its own answer but hereâÂÂs a mini-review: âÂÂ
SmartPtr
â is a bad name. It doesnâÂÂt tell the user what it actually does. Choose a name thatâÂÂs descriptive, such asSharedPtr
orRefCountedPtr
.â Konrad Rudolph
Aug 1 at 16:36