«

Hi there

I have implemented a very simple smartpointer class (invasive
design). And I was wondering what should be the natural API when using
those in a Container.

I choose to define the following operator in my smartpointer class:

....
operator ObjectType * () const
{ return Pointer; }
....

which in turn forces me to have a pointer interface (instead of const
reference):

class Container {
public:
Container():Instance(0) {}
void Set(Object *o) { Instance = o; } // Pointer interface
private:
SmartPointer<Object> Instance;
};


I would have preferred a const reference interface, such as:

class Container {
public:
Container():Instance(0) {}
void Set(Object const &o) { Instance = const_cast<Object*>(&o); }
private:
SmartPointer<Object> Instance;
};

Should I refactor my smartpointer class to be more reference oriented
instead of pointer oriented. I do not see a case where the smart
pointer should point to a NULL pointer.

Comments ?

thanks
-Mathieu


// Full code:
#include <assert.h>

template<class ObjectType>
class SmartPointer
{
public:
SmartPointer()ointer(0) {}
SmartPointer(const SmartPointer<ObjectType>& p)ointer(p.Pointer)
{ Register(); }
SmartPointer(ObjectType* p)ointer(p)
{ Register(); }
~SmartPointer() {
UnRegister();
Pointer = 0;
}
ObjectType *operator -> () const
{ return Pointer; }
operator ObjectType * () const
{ return Pointer; }
void operator = (SmartPointer const &r)
{ return operator = (r.GetPointer()); }
void operator = (ObjectType *r)
{
if(Pointer) Pointer->UnRegister();
Pointer = r;
if(Pointer) Pointer->Register();
}
private:
void Register() { if(Pointer) Pointer->Register(); }
void UnRegister() { if(Pointer) Pointer->UnRegister(); }

ObjectType* Pointer;
};

class Object
{
public:
Object():ReferenceCount(0) {}
virtual ~Object() { assert(ReferenceCount == 0); }

void Register() {
ReferenceCount++;
}
void UnRegister() {
ReferenceCount--;
if(!ReferenceCount) delete this;
}
private:
long ReferenceCount;
};

class Foo : public Object
{
public:
int F;
};

class Container {
public:
Container():Instance(0) {}
void Set(Object *o) { Instance = o; }
private:
SmartPointer<Object> Instance;
};

int main()
{
SmartPointer<Foo> o = new Foo;
Container c;
c.Set( o );
return 0;
}

mathieu wrote:
> class Object
> {
> public:
> Object():ReferenceCount(0) {}
> virtual ~Object() { assert(ReferenceCount == 0); }
>
> void Register() {
> ReferenceCount++;
> }
> void UnRegister() {
> ReferenceCount--;
> if(!ReferenceCount) delete this;
> }
> private:
> long ReferenceCount;
> };
>
> class Foo : public Object
> {
> public:
> int F;
> };

You have a basic problem with your reference-counting base class there.

Assume you have one instance of Foo allocated behind one SmartPointer
(let's name it fooPtr1). The reference count of that Foo instance will be 1.
Also assume you have another instance of Foo allocated and shared
between two SmartPointers (let's name them fooPtr2 and fooPtr3). The
reference count of this instance will be 2.

Now assume you do a "*fooPtr1 = *fooPtr2;", which is not in any way a
far-fetched thing to do. What happens?

What happens is that now the object behind fooPtr1 will have its
reference counter set to 2, even though only one smart pointer is
pointing to it. When fooPtr1 is destroyed, it will decrement that
counter to 1, not destroy the object, and you have a memory leak.

Assume the inverse situation: "*fooPtr2 = *fooPtr1;" What happens?

What happens is that now the second object will have its reference
counter set to 1 even though two smart pointers are pointing to it. When
one of then is destroyed, the other will point to freed memory. Kaboom.

The solution to this problem is rather simple, though.

On Aug 28, 9:56*pm, Juha Nieminen <nos...@thanks.invalid> wrote:
> mathieu wrote:
> > class Object
> > {
> > public:
> > * Object():ReferenceCount(0) {}
> > * virtual ~Object() { assert(ReferenceCount == 0); }
>
> > * void Register() {
> > * * ReferenceCount++;
> > * }
> > * void UnRegister() {
> > * * ReferenceCount--;
> > * * if(!ReferenceCount) delete this;
> > * * }
> > private:
> > * long ReferenceCount;
> > };
>
> > class Foo : public Object
> > {
> > public:
> > * int F;
> > };
>
> * You have a basic problem with your reference-counting base class there.
>
> * Assume you have one instance of Foo allocated behind one SmartPointer
> (let's name it fooPtr1). The reference count of that Foo instance will be 1.
> * Also assume you have another instance of Foo allocated and shared
> between two SmartPointers (let's name them fooPtr2 and fooPtr3). The
> reference count of this instance will be 2.
>
> * Now assume you do a "*fooPtr1 = *fooPtr2;", which is not in any way a
> far-fetched thing to do. What happens?
>
> * What happens is that now the object behind fooPtr1 will have its
> reference counter set to 2, even though only one smart pointer is
> pointing to it. When fooPtr1 is destroyed, it will decrement that
> counter to 1, not destroy the object, and you have a memory leak.
>
> * Assume the inverse situation: "*fooPtr2 = *fooPtr1;" What happens?
>
> * What happens is that now the second object will have its reference
> counter set to 1 even though two smart pointers are pointing to it. When
> one of then is destroyed, the other will point to freed memory. Kaboom.
>
> * The solution to this problem is rather simple, though.

Move the cstor/dstor of Object to the protected: section ?
I realized that only now, but that does not work for derived class.

Any other suggestion I might be missing ?

Thanks
-Mathieu

mathieu wrote:
> Any other suggestion I might be missing ?

Yes: Implement a copy constructor and assignment operator in your
reference-counting base class which do *not* copy the reference counter
member variable. (If there's nothing else in the base class, then simply
make empty implementations of them.)

This way you can freely copy/assign derived objects without messing up
the reference counters.

On Aug 29, 12:39 am, Juha Nieminen <nos...@thanks.invalid> wrote:
> mathieu wrote:
> > Any other suggestion I might be missing ?
>
> Yes: Implement a copy constructor and assignment operator in your
> reference-counting base class which do *not* copy the reference counter
> member variable. (If there's nothing else in the base class, then simply
> make empty implementations of them.)
>
> This way you can freely copy/assign derived objects without messing up
> the reference counters.

It indeed solve the issue you reported, but does not solve the issue
where one would do:

Foo f = *fooPtr3;

Thanks again
-Mathieu

mathieu wrote:
> It indeed solve the issue you reported, but does not solve the issue
> where one would do:
>
> Foo f = *fooPtr3;

I don't see what's wrong with that.

On Aug 29, 4:45*pm, Juha Nieminen <nos...@thanks.invalid> wrote:
> mathieu wrote:
> > It indeed solve the issue you reported, but does not solve the issue
> > where one would do:
>
> > *Foo f = *fooPtr3;
>
> * I don't see what's wrong with that.

<quote>
Yes: Implement a copy constructor and assignment operator in your
reference-counting base class which do *not* copy the reference
counter
member variable. (If there's nothing else in the base class, then
simply
make empty implementations of them.)
</quote>

Which lead to:

Object(const Object&){ } // see how ReferenceCount is *never*
initialized
void operator=(const Object&){ }

Thus when you do:

Foo f = *fooPtr3;
// f.ReferenceCount is pretty much random memory

thanks
-Mathieu

On Aug 31, 10:28*am, mathieu <mathieu.malate...@gmail.com> wrote:
> On Aug 29, 4:45*pm, Juha Nieminen <nos...@thanks.invalid> wrote:
>
> > mathieu wrote:
> > > It indeed solve the issue you reported, but does not solve the issue
> > > where one would do:
>
> > > *Foo f = *fooPtr3;
>
> > * I don't see what's wrong with that.
>
> <quote>
> Yes: Implement a copy constructor and assignment operator in your
> reference-counting base class which do *not* copy the reference
> counter
> member variable. (If there's nothing else in the base class, then
> simply
> make empty implementations of them.)
> </quote>
>
> Which lead to:
>
> * Object(const Object&){ *} // see how ReferenceCount is *never*
> initialized
> * void operator=(const Object&){ *}
>
> Thus when you do:
>
> * Foo f = *fooPtr3;
> * // *f.ReferenceCount is pretty much random memory
>
> thanks
> -Mathieu

Alright found the answer in:

http://www.parashift.com/c++-faq-lit...html#faq-16.24

And copy cstor need to initialize the ref count:

Object(const Object&):ReferenceCount(0){}
void operator=(const Object&){ }

Thanks the ref counting is much user friendly !

-Mathieu

mathieu wrote:
> <quote>
> Yes: Implement a copy constructor and assignment operator in your
> reference-counting base class which do *not* copy the reference
> counter
> member variable.

I said you should not copy the reference counter. I didn't say you
shouldn't initialize it.