C++ Smart Pointer Question

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

---