C++ Smart Pointer Question

Hello ,
I'm a little rusty on c++ so i want to ask a questions to the gurus.
I am trying to create a smart pointer(dont wanna use boost) and i want to know if the following implementation is ok :

Code:

---


#ifndef SMARTPTR_H_
#define SMARTPTR_H_

#ifdef __DEBUG__SESSION
#include <stdio.h>
#endif

namespace smartptr {

inline void DEBUG(const char *msg) {
#ifdef __DEBUG__SESSION
 printf("%s\n",msg);
#endif
}

template<typename T>
class SmartPtr {
 T *m_ptr;
 bool m_isCollection;
 int *m_refCount;
 int temp;

public:

 SmartPtr() :
  m_refCount(0), m_ptr(0), m_isCollection(0) {
  DEBUG("Default constructor.");
 }

 SmartPtr(T* ptr, bool isCollection = false) :
  m_refCount(0) {
  DEBUG("T* constructor.");
  reset(ptr, isCollection);
 }

 SmartPtr(const SmartPtr &other) :
  m_refCount(0) {
  reset(other.m_ptr, other.m_isCollection);
  other.release();
  DEBUG("Copy constructor.");
 }

 T* get() const {
  DEBUG("GET.");
  return m_ptr;
 }

 bool isValid() const {
  return (m_ptr != 0);
 }

 void reset(T* ptr, bool isCollection = false) {
  destroy();
  m_refCount = &temp;
  m_ptr = ptr;
  m_isCollection = isCollection;
  *m_refCount = 1;
  DEBUG("Reset.");
 }

 T* release() const {
  T* ptr = m_ptr;
  if (m_refCount) {
  (*m_refCount)++;
  }
  DEBUG("Released pointer ownership.");

  return ptr;
 }

 void destroy() {

  if (m_ptr && m_refCount) {
  (*m_refCount)--;
  if (*m_refCount == 0) {
    if (m_isCollection) {
    delete[] m_ptr;
    m_ptr = 0;
    DEBUG("Destroyed collection.");
    return;
    }

    delete m_ptr;
    m_ptr = 0;
    DEBUG("Destroyed single pointer.");
  }
  }

  if (m_refCount != 0) {
  m_refCount = 0;
  }
 }

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

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

 const SmartPtr& operator=(T* ptr) {
  reset(ptr);
  DEBUG("Op = T*.");
  return *this;
 }

 const SmartPtr& operator=(const SmartPtr<T> &other) {
  DEBUG("Equals");
  reset(other.m_ptr, other.m_isCollection);
  other.release();
  return *this;
 }

 ~SmartPtr() {
  destroy();
 }

};

}

#endif /* SMARTPTR_H_ */

---

I need to know especially the part where the reference counter is involved. I want to keep everything in the class on the stack so the m_refCount to a stack location. Is this illegal and if yes what are the dangers. Thx in advance.

Quote:

---

Originally Posted by gioni_go

I am trying to create a smart pointer(dont wanna use boost) and i want to know if the following implementation is ok :

---

I strongly advice against implementing your own smart pointer. They're notoriously difficult to get right.

Why don't you want to use the smart pointers of boost?

you can as well use the smart pointers in std::tr1, if your compiler is up-to-date.

These are not the answers i want to hear. Please answer to the question i asked , i already said i dont want to use boost or any other implementation.

One problem I see is that the access to the refcount does not seem to be thread-safe because it is not protected b a lock or something. I still stick to my advise not to re-invent the wheel and use something that is known to be working good.

Quote:

---

Originally Posted by gioni_go

These are not the answers i want to hear. Please answer to the question i asked , i already said i dont want to use boost or any other implementation.

---

Yes, we just think that's a very silly attitude and would like you to explain it.

I am doing some programming for Symbian and i don't want to use the cleanup-stack stuff seems rather cumbersome and non portable. I just want a simple smart pointer , no need for thread safety , i am not using threads. I am not sure if there is an alternative, but i wanted a smart pointer that uses internally only the stack , no heap allocations. I created this simple one. Thx for your answers.

Quote:

---

Originally Posted by gioni_go

These are not the answers i want to hear. Please answer to the question i asked , i already said i dont want to use boost or any other implementation.

---

In that case I suggest you contact a consulting firm.

If you want any help from me I first want to know why you cannot use a smart pointer from boost or std/tr1. But you don't have to tell me because I already know from your reaction. This is homework and you're not allowed to use any of those.

I'm not sure it's possible to do reference-counting without a heap allocation. At least, not without introducing considerable unnecessary complexity.

Quote:

---

Originally Posted by gioni_go

I am not sure if there is an alternative, but i wanted a smart pointer that uses internally only the stack , no heap allocations.

---

Your best option is to use intrusive_ptr from boost. You put the reference counter in the object. It means it will be located where the object is located, on the stack or on the heap. You don't have to make the counter threadsafe if you don't want to. It's up to you to increment/decrement the counter. Just make sure you don't delete the object if it wasn't allocated with new.

Piece of cake really and you get a robust and well tested smart pointer implementation for free.

I don't think using the data member 'temp' as a reference counter will work.

There are lots of open source reference counting classes out there, why not lift code from one of those? wxWidgets has a nice one.

Quote:

---

Originally Posted by gioni_go

Is this illegal and if yes what are the dangers. Thx in advance.

---

There's nothing wrong, technically, to have m_refCount(0) point to the count variable declared on the stack. But the problem with this approach is about ten-fold.
The "danger" lies in the fact that having m_refCount on the stack instead on the heap,
defeats the very purpose of reference counting of temp variable in your SmartPtr class.
Copying m_refCount will still point to the temp, but the copying of temp will not result in correct count.
In simple words,
your SmartPtr class itself needs another smart pointer semantic inside which you did not implement.
The difficulty of implementing such semantic is the complexity noted by Lindley.
(I personally have not tried it, but you can make an educated guess how hard it would be)

Code:

---

T* release() const {
  T* ptr = m_ptr;
  if (m_refCount) {
  (*m_refCount)++;
  }
  DEBUG("Released pointer ownership.");

  return ptr;
 }

---

Why are you thinking of releasing the ownership to who knows where?
That alone doesn't make sense at all.
Assuming release() will be used by SmartPtr only is a big mistake

there are other problems.
illogical order of the all-in-one call chain to ctor, reset, destroy.
incomplete implementation of copy ctor, operator=()
(all these methods themselves have problems of their own).
bottom line is that your SmartPtr class needs a complete make over as a whole.

If your c++ is little rusty,
I'd suggest that you try much simpler smart pointer class
and plainly have reference count variable on the heap.

Quote:

---

Originally Posted by potatoCode

But the problem with this approach is about ten-fold.

---

Admittedly, smart pointers traditionally are used to manage heap allocated objects, but that's only by convention.

Nothing prevents you from having reference counting on stack allocated objects really. It's just that you cannot delete such objects.

So the gut reaction that reference counting for stack allocated objects is wrong, is misdirected in my view. This is why we are using C++ in the first place, to have memory management options.

And the easiest way to achieve this to use the intrusive_ptr of boost (see my previous post). It gives you great flexibility in where to place the reference counter and what actions to take when it changes.

Quote:

---

Originally Posted by nuzzle

Admittedly, smart pointers traditionally are used to manage heap allocated objects, but that's only by convention.

Nothing prevents you from having reference counting on stack allocated objects really. It's just that you cannot delete such objects.

---

I agree and that's very open minded perspective.

Although unrelated to this issue,
I once (when I was a really really newbie xD) tried to design factory pattern without using new.
And most of the members advised against it or thought it cann't be done.
Having had this valuable experience,
I was able keep my focus on addressing the problems the OP had,
rather than simply come out and tell him "it's a bad idea."

Quote:

---

Originally Posted by potatoCode

I agree and that's very open minded perspective.

Although unrelated to this issue,
tell him "it's a bad idea."

---

Okay, I shouldn't have referred to your post in my reply.

You never said reference counting of stack allocated objects was bad, you merely (successfully) pointed at the potential problems.

I'm sorry to have implicated you.

Quote:

---

Originally Posted by nuzzle

Okay, I shouldn't have referred to your post in my reply.

You never said reference counting of stack allocated objects was bad, you merely (successfully) pointed at the potential problems.

I'm sorry to have implicated you.

---

Nah, don't be.
I can sense that behind those bold and gutsy words of yours lies a deep well of insights and knowledge.
Believe it or not, I learn much from your posts.

Quote:

---

Originally Posted by potatoCode

Nah, don't be.
I can sense that behind those bold and gutsy words of yours lies a deep well of insights and knowledge.
Believe it or not, I learn much from your posts.

---

Thank you.

I try to be as much no-nonsense as I can but sometimes I go overboard and become downright ugly. I know that.

The reason is that when I first started to post at different forums I didn't know English too well and I got constantly abused in discussions. I decided to learn English well enougth to reply back to whatever stupidities were thrown at me. Now when I see a chance to get even for earlier beatings I take it. I tend to show no mercy.

That's my background. If you try to sit on me you're toast. It's not a promise. It's a fact.

So that's how the legend of the "Batman" was born.
It must have been hard, no question about it.

Any more comments from me will litter the thread.
So I'll just say that I'm glad you shared your story.

Quote:

---

Originally Posted by gioni_go

I am doing some programming for Symbian and i don't want to use the cleanup-stack stuff seems rather cumbersome and non portable.

---

just for curiosity (I'm not sarcastic, I'm really curious), what's non portable in boost smart pointer implementation ?

Quote:

---

Originally Posted by potatoCode

The "danger" lies in the fact that having m_refCount on the stack instead on the heap,defeats the very purpose of reference counting of temp variable in your SmartPtr class.Copying m_refCount will still point to the temp, but the copying of temp will not result in correct count.

---

Copying m_refCount ??? I dont get it why would i copy it ? I have defined the copy constructor just to ensure that this won't happen .

Quote:

---

Originally Posted by potatoCode

Why are you thinking of releasing the ownership to who knows where?
That alone doesn't make sense at all.
Assuming release() will be used by SmartPtr only is a big mistake

---

I never assumed that . release removes ownership of current SmartPtr.

Quote:

---

Originally Posted by potatoCode

incomplete implementation of copy ctor, operator=()

---

Why are they incomplete ? I don't see that .

Quote:

---

Originally Posted by superbonzo

just for curiosity (I'm not sarcastic, I'm really curious), what's non portable in boost smart pointer implementation ?

---

Exceptions and memory allocations are handled in a different way on Symbian. It would be very difficult to port boost stuff rather than make a small smart_ptr.

Quote:

---

Originally Posted by gioni_go

Exceptions and memory allocations are handled in a different way on Symbian. It would be very difficult to port boost stuff rather than make a small smart_ptr.

---

I see (although it's not clear why somehow adapting boost implementation to your case would be more difficult or risky then writing your own smart pointer).

anyway, suppose you have

Code:

---

SmartPtr<int>        pi( new int );
SmartPtr<int>        pi2( pi );
// ...
SmartPtr<int>        piN( piN_minus_1 );

---

following your code you have (dropping inessential or empty code paths)

Code:

---

pi.m_refCount = &pi.temp;
pi.m_ptr = new int;
pi.*m_refCount = 1; // pi.temp = 1

pi2.m_refCount = &pi2.temp;
pi2.m_ptr = pi.m_ptr;
pi2.*m_refCount = 1;        // pi2.temp = 1;

if( pi2.m_refCount )
{
        (*pi2.m_refCount)++; // pi2.temp = 2;
}

// ...

---

as you can see piK.m_refCount always points to piK.temp that in turn is always 2 (excluding pi.temp that is 1). Nonetheless the code appears to work because pi is constructed first. If you can guarantee the order of your smartptr constructions then you don't need smart pointers anyway (a simpler boost:scoped_ptr and regular pointers will do) and if you can't then your code will fail. ( of course, if I read your code well :) )

Quote:

---

Originally Posted by superbonzo

Nonetheless the code appears to work because pi is constructed first.

---

Hmm that is interesting , could you give me an example in this direction, or at least what do you mean by that ?
My code would probably look pretty much like this most of the time.

{
smartptr::SmartPtr<CSomeClass> cls = CSomeClass::NewL();
cls->InvokeMethodThatMightLeave();

.....
}

The counter was made for situations like these:

{
smartptr::SmartPtr<CSomeClass> cls = CSomeClass::NewL();
smartptr::SmartPtr<CSomeOtherClass> inst = cls->InvokeMethodThatMightLeave();

.....
}

In this manner the pointer tested seemed to work ok. No problem so far.

if no concurrency is involved and you can guarantee that all SmartPtr's are stack allocated (this includes also objects containing a SmartPtr) then every SmartPtr instance will be in one and only one scope. This implies that the first SmartPtr constructed is also the last SmartPtr destroyed.

In this situation your code accidentally works.
The problem is that in this situation there's no need of reference counting. You could use any RAII construct like std::auto_ptr or boost::scoped_ptr.

Otherwise it's easy to write leaking code using your SmartPtr's:

Code:

---

class A
{
public:
        A(): pi_( new int ) {}
        A(const A& a): pi_( a.pi_ ) {}

private:
        SmartPtr<int>        pi_;
};

// somewhere ...

A* pa = new A();
A* pa2 = new A(*pa);

// somewhere else...

delete pa;
delete pa2; // the int pi_ points to will not be deleted here

---

ps: again, I've simply quickly read your code so you should test the code above yourself...

Quote:

---

Originally Posted by gioni_go

Copying m_refCount ??? I dont get it why would i copy it ? I have defined the copy constructor just to ensure that this won't happen .

---

I said copying of, not when you copy it.
when SmartPtr gets copied, m_refCount gets copied,
and when m_refCount gets copied, that is called copying of m_refCount.

To cut to the chase, this SmartPtr class is down right wrong.
1. It fails to delete and has all the pitfalls of memory leaks
2. the value of temp is inconsistent, thus yeilds incorrect referencing counting
3. this class is not guarded against self assignment, thus is incomplete
4. copy ctor fails to initialize all the members in the initialize list, thus, again incomplete.
5. creating a pointer to T, just to have it assigned to m_ptr and returning it is redudant
6. reference counting implements RAII, managment is done inside the class,
not outside the way this class is designed.
7. returning a handle that could change the ownership without clear transfer is stupidly problematic
8. the transfer is already done in your copy ctor, why do you need an extra overhead?
9. why do you think there are so many different types of smart pointers?
This SmartPtr could be called SmartAutoPtr. Do one thing and do it well.
10. Do you still not understand why your current SmartPtr class needs another smart pointer inside this class?

Code:

---

// Assume GetTemp returns the value of temp
int main()
{
    using smartptr::SmartPtr;
    {
        SmartPtr<int> s1(new int(666));
        {
            SmartPtr<int> s2(s1);
            std::cout << "s2 temp " << s2.GetTemp() << "\n";
        }
        std::cout << "s1.temp " << s1.GetTemp() << "\n";
    }
}

---

I'd have told you in more details, to the best of my knowledge,
why your copy controls were incomplete.
but you talk like you've foreseen what I had to offer, so I rather not.

Quote:

---

Originally Posted by superbonzo

if no concurrency is involved and you can guarantee that all SmartPtr's are stack allocated (this includes also objects containing a SmartPtr) then every SmartPtr instance will be in one and only one scope. This implies that the first SmartPtr constructed is also the last SmartPtr destroyed.

---

This is true , there is no concurrency involved with the SmartPtr and it is always stack allocated. But i would also need to pass it as function/method return and as function/medthod parameter, thus the counter need. I dont wanna use plain pointers.

Quote:

---

Originally Posted by superbonzo

In this situation your code accidentally works.
The problem is that in this situation there's no need of reference counting. You could use any RAII construct like std::auto_ptr or boost::scoped_ptr.

Otherwise it's easy to write leaking code using your SmartPtr's:

Code:

---

class A
{
public:
        A(): pi_( new int ) {}
        A(const A& a): pi_( a.pi_ ) {}

private:
        SmartPtr<int>        pi_;
};

// somewhere ...

A* pa = new A();
A* pa2 = new A(*pa);

// somewhere else...

delete pa;
delete pa2; // the int pi_ points to will not be deleted here

---

ps: again, I've simply quickly read your code so you should test the code above yourself...

---

Hmm not sure if accidentally but the code above works. Again i cannot use stl or boost the platform does not support them and it would take more time to port them. Anyway thx for the advice.

Quote:

---

Originally Posted by potatoCode

I said copying of, not when you copy it.
when SmartPtr gets copied, m_refCount gets copied,
and when m_refCount gets copied, that is called copying of m_refCount.

To cut to the chase, this SmartPtr class is down right wrong.
1. It fails to delete and has all the pitfalls of memory leaks
2. the value of temp is inconsistent, thus yeilds incorrect referencing counting
3. this class is not guarded against self assignment, thus is incomplete
4. copy ctor fails to initialize all the members in the initialize list, thus, again incomplete.
5. creating a pointer to T, just to have it assigned to m_ptr and returning it is redudant
6. reference counting implements RAII, managment is done inside the class,
not outside the way this class is designed.
7. returning a handle that could change the ownership without clear transfer is stupidly problematic
8. the transfer is already done in your copy ctor, why do you need an extra overhead?
9. why do you think there are so many different types of smart pointers?
This SmartPtr could be called SmartAutoPtr. Do one thing and do it well.
10. Do you still not understand why your current SmartPtr class needs another smart pointer inside this class?

Code:

---

// Assume GetTemp returns the value of temp
int main()
{
    using smartptr::SmartPtr;
    {
        SmartPtr<int> s1(new int(666));
        {
            SmartPtr<int> s2(s1);
            std::cout << "s2 temp " << s2.GetTemp() << "\n";
        }
        std::cout << "s1.temp " << s1.GetTemp() << "\n";
    }
}

---

I'd have told you in more details, to the best of my knowledge,
why your copy controls were incomplete.
but you talk like you've foreseen what I had to offer, so I rather not.

---

Ok so i have tested the code above and the smart pointer works ok.

About the points you enumerated.
1. Please explain in more detail maybe i'm dumb or so but i don't see why it fails to delete the pointer.
2. Why is it inconsistent ? Details please :).
3. Me dumb here , fixed that.
4. Why would i need to initialize all memebers ? I only initialize the ones that have to be so. temp is not really needed to be initialized since it is assigned in reset.
5. Dumb i know, fixed that.
6. Hmm , details again, can't see the problem here.
7. Hmm , details again, can't see the problem here.
8. ?????
9. It is intended to be just a simple auto_ptr but for the reasons i have enumerated above i added a counter.
10. Partially but not convinced.

Please do not take my words as an insult but i haven't worked with c++ for a long time and i want to comprehend hopefully some of the concepts. I hope i do not become annoying.

From the code above now i have seen the problem:

int main() {

using smartptr::SmartPtr;
{
SmartPtr<int> s1(new int(666));
{
SmartPtr<int> s2(s1);
SmartPtr<int> s3(s1);
}

}

return 0;
}

This would crash.

You should also ensure that the following test behaves appropriately:

Code:

---

using smartptr::SmartPtr;

int main()
{
    SmartPtr<int> s2;
    {
        SmartPtr<int> s1(new int(5));
        s2 = s1;
    }
    SmartPtr<int> s3(s2);

    // Verify s3 points to an int with the value 5
   
    return 0;
}

---