Never ever use a raw pointer when a smart pointer can do the same job

[Noah Roberts]

(just-kidding(

Hmm ... interesting you changed "colleague" to "boss".

I misread


I wonder if

 

[Noah Roberts]

I have written "delete this" but I certainly would not classify it as
typical either. Certainly, some specific problems can have a very
elegant solution using "delete this" but I see it as one possible
solution to limited set of situations.

And I've seen it used too much and debugged too much code that used it.
MUCH care need be taken when you do a delete this. For one, you
better have that constructor protected so naive coders don't think it's
part of the public interface and just try to use it.

In general, I don't like the idea of an object being responsible for its
own lifetime. There are always exceptions to any general rule, so
before the pedants tear into me for saying so: yes, there are
exceptions. In general though, an object should have one responsibility
only and if all an object is responsible for is its own lifetime...then
it has no reason to exist.

 

[Noah Roberts]

Do they? boost::shared_ptr certainly doesn't.

#include <iostream>
#include <boost/shared_ptr.hpp>

struct test_object
{
~test_object() { std::cout << "object deleted\n"; }
};

int main()
{
boost::shared_ptr<test_object> ptr(new test_object);

ptr.reset(); // delete the object explicitly

std::cout << "post reset\n"; // to demonstrate it wasn't scope.
}

 

[Noah Roberts]

But that's irrelevant, since they don't provide the interface or
the services of a pointer.

Sure they do. When applied to a particular use of pointers: as
addresses to arrays, the std::vector object very much does provide this
service. Any function that accepts a pointer to T and then treats that
pointer as an array can be modified to accept a std::vector simply by
changing the parameter. However, it does this "smartly" and there is
much more information available from the std::vector than there is from
the pointer.

There is no smart pointer that can or should provide ALL the services a
pointer can be used to provide.

 

[Noah Roberts]

#include <iostream>
#include <boost/shared_ptr.hpp>

struct test_object
{
~test_object() { std::cout << "object deleted\n"; }
};

int main()
{
boost::shared_ptr<test_object> ptr(new test_object);

ptr.reset(); // delete the object explicitly

std::cout << "post reset\n"; // to demonstrate it wasn't scope.
}

You probably mean when there's more than one shared ptr to the item
though. This is not something shared_ptr can or should do. If you want
a pointer that can be destroyed and notify all referrers that the
pointer is destroyed then you need something else.

There are many smart pointers.

 

[James Kanze]

Sure they do.

There must be some degree of misunderstanding here. The
interface of a pointer consists of a small number of operations:
unary *, ->, and the various addition and subtraction operations
(+, +=, ++, -, -=, and --), plus the [] operator, which is
mapped into + followed by *. The only one of these supported by
std::vector is []. Most smart pointers (intelligently) don't
support the addition and subtraction operators, but from what
I've always understood, the definition of a "smart pointer" is a
class which supports unary * and ->. Those seem to be the
operations which characterize a pointer in most people's minds.

When applied to a particular use of pointers: as addresses to
arrays, the std::vector object very much does provide this
service.

OK. I see what you mean. In cases where you'd write something
like:

T* array = new T[ n ] ;
// use array as an array, even though it is a pointer.
delete [] array ;

, you can (and should) replace the T* with std::vector< T >.
I've just never thought of this as "using a pointer"; in my
mind, it's not so much a case of std::vector emulating a
pointer, as one of pointers in C emulating an array. (I've
always considered this a major design flaw of C, but that's
neither here nor there.) To me, in this case, it doesn't feel
like I'm using a pointer, so I don't think of its replacement as
a smart pointer. (Of course, I've never actually written
anything like this in C++---one of the first classes I ever
wrote was ArrayOf. Back when I was working in C, such code,
using malloc/free instead of new/delete, was common, however.
But even then, I just didn't think of it as a pointer.)

Any function that accepts a pointer to T and then
treats that pointer as an array can be modified to accept a
std::vector simply by changing the parameter. However, it
does this "smartly" and there is much more information
available from the std::vector than there is from the pointer.

Agreed. I just think of this as an "array" with implicit
reference semantics, even in C.

There is no smart pointer that can or should provide ALL the
services a pointer can be used to provide.

Certainly not. One of the advantages of smart pointers is that
you can't use pointer arithmetic on them. And that smart
pointers and arrays are two different, more or less unrelated
things (with no implicit conversion of arrays to pointers, and
no possibility to use a pointer as an array).

In the end, it's a question of vocabulary. We both pretty much
agree that dynamically allocated arrays should usually be
replaced with std::vector. My impression is that in most
people's minds, "smart pointer" is associated with classes that
support unary* and -> (like std::vector<>::iterator), and not
with other classes. But it is admittedly a pretty arbitrary
definition. (Unless, like me, you think that the idea of
pointers supporting [] a dumb idea, so that anything which
emulates that can't be "smart".)

 

[James Kanze]

And I've seen it used too much and debugged too much code that
used it. MUCH care need be taken when you do a delete this.
For one, you better have that constructor protected so naive
coders don't think it's part of the public interface and just
try to use it.

I suspect that there's a typo in there somewhere---you *never*
use delete this in a constructor. For the rest, delete this
doesn't require more care than any other delete. Regardless of
where the delete comes from, you have to ensure that all other
concerned parties are notified.

In general, I don't like the idea of an object being
responsible for its own lifetime.

I'd consider it a fundamental concept of OO---an object has
behavior, and if the behavior required for a specific message is
to terminate the object's lifetime, delete this seems to me the
simplest, clearest and most easily understood means of doing so.
There are obviously many cases where it is not
appropriate---transactions come to mind (where the "termination"
may have to be rolled back), or GUI code with separation of view
and model (where the model treats the event, and terminates the
lifetime of the view). But there are just as many where it is
appropriate.

There are always exceptions to any general rule, so before the
pedants tear into me for saying so: yes, there are exceptions.
In general though, an object should have one responsibility
only and if all an object is responsible for is its own
lifetime...then it has no reason to exist.

.

An object is responsible for handling events which concern it;
if the event which concerns it calls for its deletion, who
better to do the delete. An object is responsible for managing
its state; whether it exists or not is part of its state.

And so on. In practice, each application is a case of its own,
and the decision will depend in many ways on the structure of
your code. There are certainly a lot of situations where delete
this is not appropriate---generally more or less for the reason
you mentionned (e.g. a GUI object generally won't handle its
events directly, since its responsibility is managing the
display; it will pass them on to an event handler, which will
delete the GUI object---and perhaps indirectly itself, since
it's quite conceivable that the GUI object delete all of the
event handlers it owns in its destructor).

Anyway, I won't insist too much on the delete this, since it
really is incidental. The point is that somewhere, someone will
want to delete the object explicitly, in reaction to an external
event, and that in many cases, the pointer that that someone has
will derive from the this pointer of the object (and smart
pointers which don't work if you create new instances from the
this pointer, like boost::shared_ptr, are simply banned in
projects where I have a say).

 

[James Kanze]

Despite the relative publicity around reference counted
pointers, I personally, think that the standard commitee got
it right when they pick auto_ptr for their only (first?) smart
pointer. I find passing of ownership more often useful than
refence counting. But I certainly wouldn't classified them as
"least useful"

Since you are coming out so strongly against boost:shared_ptr,
would you mind giving examples of other types of pointers you
find more useful and clarify why you feel boost:shared_ptr
have so little usefulness in them.

The one I use most is std::auto_ptr. Second (but a very distant
second) is my own reference counted pointer, which is invasive.
Application specific smart pointers are probably more frequent
than either of these, but it's each time a different pointer
type.

Boost::shared_ptr has a very serious flaw: you can't create two
of them from the same raw pointer without getting into trouble.
And of course, the pointer you have most frequently access to is
the this pointer, and that it a raw pointer. Which makes
boost::shared_ptr extremely error prone (for managing
lifetime---I have no problems with using it with an explicit
deleter that doesn't terminate the lifetime of the object).

For boost::shared_ptr to be used safely for object lifetime
management, cycles must be impossible (i.e. the pointed to
object contains no pointers), and all pointers to the object
must be boost::shared_ptr (including the this pointer). Since
the first condition is not all that frequent, and the second is
impossible in C++, the only reasonable solution is to ban the
class (again, when used for object lifetime management).

 

[James Kanze]

[...]

#include <iostream>
#include <boost/shared_ptr.hpp>

struct test_object
{
~test_object() { std::cout << "object deleted\n"; }
};

int main()
{
boost::shared_ptr<test_object> ptr(new test_object);

ptr.reset(); // delete the object explicitly

std::cout << "post reset\n"; // to demonstrate it wasn't scope.
}

But you're not sharing anything, so it's not really an example
of using a shared_ptr; i.e. reset() will delete the object in
cases where boost::scoped_ptr would have made more sense.

The point is that in a large application, processing external
events in more or less real time (i.e. a server or a GUI), the
lifetime of most dynamically allocated objects should be
logically terminated in response to an external event---if I
have a dialog box popped up, and someone clicks on the X in the
corner, the dialog box object should cease to exist, regardless
of who has a pointer to it or not. The "correct" behavior isn't
to keep it alive until the last pointer to it disappears, but to
notify all objects which have a pointer to it (in its
destructor), so that they can get rid of the pointer (and
perhaps take additional actions).

This is a real problem, and it does result in program errors.
Using boost::shared_ptr here, however, may give a false sense of
security, but it doesn't solve the problem (and creates other
problems of its own). In all of the applications I've worked
on, we've used application specific observers---maybe some sort
of smart pointer which requires the association of an observer
would be an answer, but to date, I've not seen an implementation
of anything which really works.

 

[Noah Roberts]

There must be some degree of misunderstanding here.
Obviously.

One of the advantages of smart pointers is that
you can't use pointer arithmetic on them.

That's only true of a subset of smart pointers. Since all iterators are
smart pointers the count of such that support (nay, are built for)
pointer arithmetic in the standard vastly outnumber the ones that do
not...even C++0x.

A smart pointer provides some subset of the functionality of raw
pointers and limits and/or extends the interface to provide for a
particular use case.

 

[Noah Roberts]

I suspect that there's a typo in there somewhere---you *never*
use delete this in a constructor. For the rest, delete this
doesn't require more care than any other delete.

Oh really? Well, let's try out this code:

struct an_object
{
void f() { delete this; }
};

void some_function()
{
an_object x;
x.f();
}

 

[Alf P. Steinbach]

* Noah Roberts:

Oh really? Well, let's try out this code:

struct an_object
{
void f() { delete this; }
};

void some_function()
{
an_object x;
x.f();
}

I guess your point is that some more care is required for 'delete this' than for
'delete that', namely, taking care not to access any members afterwards, which
can be more of a problem when one is currently within a member routine.

However your example does not illustrate that, it only illustrates the usual
dangers of 'delete' that apply irrespective of the context.

One way to design the class so that it's more safe wrt. the usual dangers is as
follows:

class AnObject
{
protected:
virtual ~AnObject() {}
public:
void f() { delete this; }
};

void someFunc()
{
AnObject x; // Compilation error, must be new'ed.
x.f();
}

Cheers,

- Alf

 

[James Kanze]

That's only true of a subset of smart pointers. Since all
iterators are smart pointers the count of such that support
(nay, are built for) pointer arithmetic in the standard vastly
outnumber the ones that do not...even C++0x.

Yes. I'm not sure I'd consider that an advantage, however.
But my point does hold for some smart pointers, which are only
designed to point to a single object.

You keep this up, and you'll convince me. The problem with raw
pointers for navigation isn't that they don't do everything
that's necessary; it's that they allow the programmer to do even
more. The real problem, I guess, is that in C, pointers are
used for too many different things: they point to an
object---what I understand when I say "pointer"---but they are
also used as surrogates for arrays. And I can't think of a
single case where you'd want both functionalities.

A smart pointer provides some subset of the functionality of
raw pointers and limits and/or extends the interface to
provide for a particular use case.

OK. That's a perfectly valid definition. The one I've
generally heard is that a smart pointer is a class that
implements unary * and ->, but yours is just as valid.

 

[James Kanze]

[...]

Oh really? Well, let's try out this code:

struct an_object
{
void f() { delete this; }
};

void some_function()
{
an_object x;
x.f();
}

And how is that different from, say:

struct AnObject
{
} ;

void f( AnObject* p )
{
delete p ;
}

void someFunction()
{
AnObject x ;
f( &x ) ;
}

?

In practice, I've not found this (delete of an object on the
stack) to be much of a problem. Largely speaking (with some
notable exceptions---especially in very low level code, but
also, at times, for reasons of optimization), a given type is
either always allocated dynamically, or never; obviously, those
which are never allocated dynamically aren't deleted, and those
which are always allocated dynamically are deleted.

 

[Alf P. Steinbach]

* Yannick Tremblay:

Indeed. Or even make the constructor private and force the use of
factories.

I have never seen the great attraction of the factory approach. It is
recommended by the FAQ, but I think out of sheer conventionalism. For N classes
with an average number of M constructors it requires you to write N*M factory
functions, as opposed to just 1 common general deleter for the protected
destructor approach (if anything). It also encourages ungood practice where
factory functions are used to patch up bad designs, e.g. adding doing pre- and
post- construction actions. And it encourages ungood design just to limit the
number of factory functions.

Factory functions have their uses, but not for ensuring dynamic allocation.


Cheers & hth.,

- Alf

 

[Francesco]

Factory functions have their uses, but not for ensuring dynamic allocation.

Oh, this is very interesting for me... in another thread - which went
deserted, for my delusion - I suggested factories exactly to ensure
dynamic allocation.

So then, you guys have understood which kind of programmer I happen to
be, haven't you? ;-)

Alf, could you please point me on the right direction about ensuring
that a class never gets instantiated as a local object?

Thank you very much.

Kind regards,
Francesco

 

[Alf P. Steinbach]

* Francesco:

Oh, this is very interesting for me... in another thread - which went
deserted, for my delusion - I suggested factories exactly to ensure
dynamic allocation.

So then, you guys have understood which kind of programmer I happen to
be, haven't you? ;-)

Alf, could you please point me on the right direction about ensuring
that a class never gets instantiated as a local object?

Thank you very much.

See my example up-thread.

I discuss the issues (also how to do other such things, related) in more detail
in my old "Pointers" tutorial.


Cheers & hth.,

- Alf

 

[Francesco]

* Francesco:


See my example up-thread.

I discuss the issues (also how to do other such things, related) in more detail
in my old "Pointers" tutorial.

Reading more carefully I realized I should have been able to work it
out by myself from the latest posts of this thread, but dropping in I
got a pointer to extra info - half-full glass philosophy ;-) - extra
info == your tutorial: http://alfps.izfree.com/tutorials/pointers/

Thanks a lot, I'll profit of it.

Best regards,
Francesco

 

[Noah Roberts]

* Noah Roberts:

I guess your point is that some more care is required for 'delete this'
than for 'delete that', namely, taking care not to access any members
afterwards, which can be more of a problem when one is currently within
a member routine.

However your example does not illustrate that, it only illustrates the
usual dangers of 'delete' that apply irrespective of the context.

One way to design the class so that it's more safe wrt. the usual
dangers is as follows:

class AnObject
{
protected:
virtual ~AnObject() {}
public:
void f() { delete this; }
};

void someFunc()
{
AnObject x; // Compilation error, must be new'ed.
x.f();
}

Which is of course exactly what I said was important to remember when
you did a "delete this" (I protected the constructor instead). You'd
better make sure nobody can create that object as a local stack item if
you're going to do that.

 

[Noah Roberts]

* Yannick Tremblay:

I have never seen the great attraction of the factory approach.

Well, for one, what if you want to change what's returned from the
constructor? There's the one case where you want to ensure that the
object is always created with a shared_ptr (as in you're using
shared_from_this), and there's the other case when you don't care but
sometime down the road someone decides that a subclass should be
returned rather than the one you're constructing. Clients don't
actually need to know this but if you didn't wrap the constructor in a
factory method or object then they are stuck changing...every single one
that created the object.

The guys at Net Objectives actually recommend doing it all the time but
they're working with languages like Java. You just can't in C++ and
even if you did (because you could I suppose return the base as a value)
it would just lose any extra information gained by subclassing. But if
you're positive that nobody should be creating some class on the stack
then it can be a very positive step to simply wrap the constructor(s) in
factory functions. They don't have to be the AbstractFactory object,
but a basic FactoryMethod can provide this service.

The abstract factory works well when you're creating a conglomeration of
objects based upon the parameters passed in. It can be the one, sole
object in your design that has to know about the whole tree of classes,
pick the best one(s), and build what clients need. That's really the
only thing I ever use it for. If I find myself implementing an NxM set
of methods in this factory, as you indicate, I ask myself if it's really
the best thing for the job.