custom smart pointer class template

The name of the pictureThe name of the pictureThe name of the pictureClash 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();







share|improve this question

















  • 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






  • 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 as SharedPtr or RefCountedPtr.
    – Konrad Rudolph
    Aug 1 at 16:36

















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();







share|improve this question

















  • 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






  • 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 as SharedPtr or RefCountedPtr.
    – Konrad Rudolph
    Aug 1 at 16:36













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();







share|improve this question













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();









share|improve this question












share|improve this question




share|improve this question








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 using std::unique_ptr or std::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 as SharedPtr or RefCountedPtr.
    – Konrad Rudolph
    Aug 1 at 16:36













  • 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






  • 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 as SharedPtr or RefCountedPtr.
    – 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











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






share|improve this answer





















  • 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 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

















up vote
7
down vote













  • As @bipll mentioned, templates 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 (*, ->) need const overloads (or need to be made const), as otherwise the contained pointer isn't be accessed from a const context, e.g. a const 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 accessing SmartPtr<T>::_ptr gets cumbersome.



  • This implementation seems similar to std::shared_ptr in the sense that the SmartPtr is intended to be shared. However, if this "being shared" involves multiple threads, then accesses to RefCount::_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.






share|improve this answer





















  • It’s not “just” unusual, it flat out breaks some build systems. Don’t do it.
    – Konrad Rudolph
    Aug 1 at 16:34

















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 within SmartPtr.


  • 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.






share|improve this answer




























    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.






    share|improve this answer





















    • 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










    Your Answer




    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: false,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );








     

    draft saved


    draft discarded


















    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






























    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






    share|improve this answer





















    • 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 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














    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






    share|improve this answer





















    • 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 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












    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






    share|improve this answer













    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







    share|improve this answer













    share|improve this answer



    share|improve this answer











    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 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
















    • 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 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















    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












    up vote
    7
    down vote













    • As @bipll mentioned, templates 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 (*, ->) need const overloads (or need to be made const), as otherwise the contained pointer isn't be accessed from a const context, e.g. a const 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 accessing SmartPtr<T>::_ptr gets cumbersome.



    • This implementation seems similar to std::shared_ptr in the sense that the SmartPtr is intended to be shared. However, if this "being shared" involves multiple threads, then accesses to RefCount::_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.






    share|improve this answer





















    • It’s not “just” unusual, it flat out breaks some build systems. Don’t do it.
      – Konrad Rudolph
      Aug 1 at 16:34














    up vote
    7
    down vote













    • As @bipll mentioned, templates 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 (*, ->) need const overloads (or need to be made const), as otherwise the contained pointer isn't be accessed from a const context, e.g. a const 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 accessing SmartPtr<T>::_ptr gets cumbersome.



    • This implementation seems similar to std::shared_ptr in the sense that the SmartPtr is intended to be shared. However, if this "being shared" involves multiple threads, then accesses to RefCount::_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.






    share|improve this answer





















    • It’s not “just” unusual, it flat out breaks some build systems. Don’t do it.
      – Konrad Rudolph
      Aug 1 at 16:34












    up vote
    7
    down vote










    up vote
    7
    down vote









    • As @bipll mentioned, templates 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 (*, ->) need const overloads (or need to be made const), as otherwise the contained pointer isn't be accessed from a const context, e.g. a const 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 accessing SmartPtr<T>::_ptr gets cumbersome.



    • This implementation seems similar to std::shared_ptr in the sense that the SmartPtr is intended to be shared. However, if this "being shared" involves multiple threads, then accesses to RefCount::_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.






    share|improve this answer













    • As @bipll mentioned, templates 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 (*, ->) need const overloads (or need to be made const), as otherwise the contained pointer isn't be accessed from a const context, e.g. a const 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 accessing SmartPtr<T>::_ptr gets cumbersome.



    • This implementation seems similar to std::shared_ptr in the sense that the SmartPtr is intended to be shared. However, if this "being shared" involves multiple threads, then accesses to RefCount::_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.







    share|improve this answer













    share|improve this answer



    share|improve this answer











    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
















    • 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










    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 within SmartPtr.


    • 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.






    share|improve this answer

























      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 within SmartPtr.


      • 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.






      share|improve this answer























        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 within SmartPtr.


        • 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.






        share|improve this answer














        • 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 within SmartPtr.


        • 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.







        share|improve this answer













        share|improve this answer



        share|improve this answer











        answered Aug 1 at 12:02









        Toby Speight

        17.1k13485




        17.1k13485




















            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.






            share|improve this answer





















            • 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














            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.






            share|improve this answer





















            • 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












            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.






            share|improve this answer













            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.







            share|improve this answer













            share|improve this answer



            share|improve this answer











            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 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
















            • 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















            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












             

            draft saved


            draft discarded


























             


            draft saved


            draft discarded














            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













































































            Popular posts from this blog

            Greedy Best First Search implementation in Rust

            Function to Return a JSON Like Objects Using VBA Collections and Arrays

            C++11 CLH Lock Implementation