Poll: Do you use smart pointers?

[Thomas J. Gritzan]

loufoque posted:


The following code works perfectly:

#include <cstddef>
#include <cstring>
#include <cassert>

char *const PrependDriveLetter(char const letter, char const *const dir)

[...]

Ok, lets play "find the problems":

#include <cstddef>
#include <cstring>
char *const AppendPath(const char* const path, const char* const dir)
{
std::size_t pathlen = std::strlen(path);
std::size_t dirlen = std::strlen(dir);

if (path[pathlen-1] == '\\')
pathlen -= 1;

char* const str = new char[pathlen + dirlen + 2];

// new string: path + '\\' + dir + '\0';

memcpy(str, path, pathlen);
str[pathlen] = '\\';
memcpy(str+pathlen+1, dir, dirlen+1);

return str;
}

#include <iostream>
int main()
{
char* const path = AppendPath(AppendPath(
AppendPath("C:\\", "first"), "second"), "third");

std::cout << path << std::endl;
delete[] path;
}

Present this code to 100 C++ programmers, and with new/cout changed to
malloc/printf to another 100 C programmers. How many will find the problem(s)?

 

[Frederick Gotham]

Thomas J. Gritzan posted:

Ok, lets play "find the problems":

Splendid.

#include <cstddef>
#include <cstring>
char *const AppendPath(const char* const path, const char* const dir)
{
std::size_t pathlen = std::strlen(path);
std::size_t dirlen = std::strlen(dir);

I would recommend defining dirlen as const.

if (path[pathlen-1] == '\\')
pathlen -= 1;

I would recommend putting the constant to the left of the equality
operator. (I'd also advocate use of the decrement operator).

if('\\' == path[pathlen-1]) --pathlen;

Actually, come to think of it, I'd probably go so far as the following in
order to enforce constness:

using std::size_t;

size_t const pathlen_ = std::strlen(path);

size_t const pathlen = pathlen_ - ('\\'==dir[pathlen_-1]);

(You might want to add two invertors before the open-parenthesis in order
to suppress a compiler warning.)

char* const str = new char[pathlen + dirlen + 2];

Let's assume for testing that:

path == "c:\\word\\"
dir == "personal"

In this case:

pathlen == 7 (after the decrement)
dirlen == 8

We now have a buffer 17 bytes long -- exactly right! I'd recommend
splitting the 2 into 1+1 though, it's more intuitive that way.

// new string: path + '\\' + dir + '\0';

memcpy(str, path, pathlen);

Absence of "std" qualifier, but OK other than that.

str[pathlen] = '\\';
memcpy(str+pathlen+1, dir, dirlen+1);

Absence of "std::" again, but OK.

return str;
}

#include <iostream>
int main()
{
char* const path = AppendPath(AppendPath(
AppendPath("C:\\", "first"), "second"), "third");

The pointer value returned by the primary and secondary invocations of
"AppendPath" are discarded, and so we've no way to deallocate the memory
allocated by them.

std::cout << path << std::endl;


delete[] path;
}

Present this code to 100 C++ programmers, and with new/cout changed to
malloc/printf to another 100 C programmers. How many will find the
problem(s)?

If you select 100 programmers at random, then 5% will be expert
programmers, 15% will be proficient programmers, and 80% will be novices.

100% of the experts will remedy all problems.
85% of proficient programmers will remedy all problems.
9% of novices will rememdy all problems.

Therefore, it is wise to be concious of one's audience when code is to be
presented to other people.

 

[Jerry Coffin]

[ ... ]

Ok, lets play "find the problems":

As the old saying goes, "REAL programmers can write FORTRAN in any
language", and IMO, this code is almost exactly that. At best it's
writing C in C++. C++ provides a perfectly good string class for
situations like this.

#include <cstddef>
#include <cstring>
char *const AppendPath(const char* const path, const char* const dir)
{
std::size_t pathlen = std::strlen(path);
std::size_t dirlen = std::strlen(dir);

if (path[pathlen-1] == '\\')
pathlen -= 1;

char* const str = new char[pathlen + dirlen + 2];

// new string: path + '\\' + dir + '\0';

memcpy(str, path, pathlen);
str[pathlen] = '\\';
memcpy(str+pathlen+1, dir, dirlen+1);

return str;
}

std::string append_path(std::string const &path, std::string const &dir)
{
std::string ret(path);
if (ret[ret.size()-1] != '\\')
ret += '\\';
ret += dir;
return ret;
}

Short, easy to understand, and virtually impossible to abuse into
leaking memory.

Present this code to 100 C++ programmers, and with new/cout changed to
malloc/printf to another 100 C programmers. How many will find the problem(s)?

Which problems? The memory leaks or the poor design or the lack of
exception safety, or ...? The next question would be who does a better
job of fixing the problems. My guess is that on the whole, the C++
programmers would do a better job of producing something like the
version above that's actually fairly robust and usable. This isn't a
matter of greater skill, but a matter of being provided tools that make
it relatively easy to do a decent job. In C you have to explicitly
manage all the memory, and explicitly free all the memory that's
allocated.

As in your example, calling free on a temporary object is essentially
impossible, meaning almost anything you do in C is relatively fragile.
Using a string class (nearly any string class, not just std::string)
makes more robust code relatively simple.

Of course, in C you can also pre-allocate the buffer and have the
function deposit the result in them:

void AppendPath(char const *path,
char const *dir,
char *result,
size_t res_length)
{
/* ... */
}

This at least prevents the obvious leak you showed, but still has major
problems. Consider, for example, if I have malice aforethought, and do
something like:

void my_exploit() {

char buffer[2];

AppendPath("A\\",
"address on the stack occupied by code I choose",
&buffer,
4096);
}

Now the code overwrites the stack frame and puts my code in place to be
executed. If AppendPath runs at a higher security level than my original
code, I've probably just breached security (in fact, code like this is a
common source of security problems).

Another method is somewhat similar, but instead of giving the address of
my own buffer (at all) what I put on the stack is the address of a
function followed by some parameters it gets called with. This is
typically done to call a function I can't call directly, or to pass
parameters that would be screened out as illegal if I called it directly
-- but if (for example) I specify an address inside of the function just
AFTER it checks parmeters...

 

[liam_herron]

Yes. When possible I try to use the stack and pass objects via
references. But, after
that I use smart ptr's. Occasionally, when I need to link structures
together, I will use
a raw ptr as a backptr (ie has no ownership). Hope that helps.

Regards,
liam

 

[Herb Sutter]

BTW, even gurus may change their mind
(http://www.gotw.ca/publications/index.htm):
"Most people have heard of the standard auto_ptr smart pointer
facility, but not everyone uses it daily. That's a shame, because it
turns out that auto_ptr neatly solves common C++ design and coding
problems, and using it well can lead to more robust code." Herb
Sutter, 1999

"Avoid using auto_ptr, instead use shared_ptr which is widely
available and being added to the standard library." Herb Sutter,
2004.

Of course, the main difference is that it became possible to recommend the
latter once we got more than just one smart pointer that fit the criteria
"is widely available and being added to the standard library."

Herb

 

[mlimber]

[...] I use smart ptr's. Occasionally, when I need to link structures
together, I will use
a raw ptr as a backptr (ie has no ownership). Hope that helps.

In the case of std::tr1::shared_ptr, that's what std::tr1::weak_ptr is
for.

Cheers! --M

 

[Earl Purple]

The FAQ includes a perfectly adequate intrusive smart pointer that I
have used for years:

http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.22

Just had a look at that. Intrusive reference-counting and their
smart-pointers, with all your classes inheriting from one class was a
popular idiom in the late 1990s. It does have its merits but it can be
trickier to write them than you think.

One thing you can't do is forwardly declare that class X derives from
Y. You can forwardly declare X but you can't say it derives from Y
which means that, from within your template, you can have problems with
the casting or calling the virtual methods of your reference-counted
base-class if you want to keep your main class (X) only
forwardly-declared.

And why can't you use boost? Does your employer not permit it?

Don't be so surprised. Most companies have policies regarding 3rd party
open-source and unfortunately boost comes under this category. Over
here we do have boost, but I have run into problems having to get
approval for some other 3rd party open-source libraries I wanted to
use.

Remember that not everyone in the C++ world is so knowledgeable about
these things.

The FAQ will start you in the right direction. Jossuttis' The C++
Standard Library has some good examples of using a non-intrusive smart
pointer (using code supplied in the book) that works well with standard
containers.

I haven't looked at the recommendations but it's an old book and I
think smart-pointers have developed a lot since then. Scott Meyers had
an intrusive smart-pointer in Effective C++ which had many defects. I'm
sure he would write a much better one now, of course.

For any reference-counted smart-pointer (whether intrusive or
non-intrusive) you should be able to do the following:

- Have an instance of one for a class that is only forwardly-declared.
- Be able to pass in a custom deleter. (For intrusive, this is done by
calling a virtual method that makes it self-delete when appropriate,
rather than your smart-pointer calling delete on the class).
- Create a SHARED_PTR<T> from a SHARED_PTR<U> where SHARED_PTR is your
shared-pointer template and whenever U is derived from T or T is const
U. Not only that but if SHARED_PTR<U> has a custom deleter then that
has to be passed into the SHARED_PTR<T> so your custom-deleter cannot
be a template class based on U that cannot convert to a template class
based on T.

Note: I have written such a smart-pointer but I borrowed a lot from
boost to do it. Years ago I wrote an intrusive one. Eventually
abandoned it.

 

[Earl Purple]

Our main product is ~80k SLOC of highly mathematical C++; there's
not a bare pointer to be seen, except in the depths of some of the
math libraries (which are not for the faint of heart).

It is not the use or raw pointers that is the problem. My code uses
them when it wants to use references but can't. It uses smart-pointers
when it needs to control lifetime.

What you don't see in my code is delete statements. All the deletes are
carried out by the smart-pointers (well actually the underlying deleter
of the smart-pointer).

Of course you have to know that the object has a lifetime longer than
any raw pointer that points to it. When you know that to be the case
(and it is quite a significant amount of the time) then use a raw
pointer if you cannot use a reference, const if appropriate.

We switched

from raw pointers to Boost's shared pointer a while back, and
haven't had a memory leak since. We rely on them completely, and
have _never_ had a problem.

The only good reason _not_ to use them AFAIK is the memory overhead
of the ref count (which can be mitigated in several ways) and the
run-time overhead of the extra redirection. We estimate that the
run-time penalty in our application for using smart pointers is
around 2%. We may, in the future, have to do something about this,
but it's unlikely.

The redirection is likely to be optimised away by any decent compiler.
The run-time overhead comes in copying them. Now if you really do have
a situation where you have to control the reference count to ensure
that the object is only deleted when the last reference to it goes
away, then you would have to do that anyway manually.

If your only need for shared_ptr is to store in containers - boost has
a fairly new ptr_container set of containers now which can be used in
its place.

If you don't want to do that, then also remember that if you are
iterating through your containers, it is cheaper to use by-reference
than by-value so your algorithms, for example, can take operator() (
/*const*/ shared_ptr< T > & ) rather than operator( shared_ptr< T > )
and remember that const above is with respect to the pointer, not the T.

 

[Thomas Tutone]

Just had a look at that. Intrusive reference-counting and their
smart-pointers, with all your classes inheriting from one class was a
popular idiom in the late 1990s. It does have its merits but it can be
trickier to write them than you think.

You're tearing down a straw man. The intrusive smart pointer in the
FAQ does not not involve any inheritance, let alone "all your classes
inheriting from one class." I described it as "perfectly adequate,"
and it is, at least for non-expert users trying to learn about smart
pointers.

One thing you can't do is forwardly declare that class X derives from
Y. You can forwardly declare X but you can't say it derives from Y
which means that, from within your template, you can have problems with
the casting or calling the virtual methods of your reference-counted
base-class if you want to keep your main class (X) only
forwardly-declared.

Since inheritance isn't involved, this isn't an issue.

Don't be so surprised.

Not to worry - I'm not surprised. That's why I asked, "does your
employer not permit it?"

Most companies have policies regarding 3rd party
open-source and unfortunately boost comes under this category. Over
here we do have boost, but I have run into problems having to get
approval for some other 3rd party open-source libraries I wanted to
use.

Remember that not everyone in the C++ world is so knowledgeable about
these things.

Agreed. In fact, I wondered if the OP simply didn't know that he could
download boost for free.

I haven't looked at the recommendations but it's an old book and I
think smart-pointers have developed a lot since then. Scott Meyers had
an intrusive smart-pointer in Effective C++ which had many defects. I'm
sure he would write a much better one now, of course.

I fear you may have missed my point. The OP asked for advice about
"the best way to make regular use of smart pointers." I responded that
Josuttis' book - still the best treatise on the Standard Library - "has
some good examples of using a non-intrusive smart pointer (using code
supplied in the book) that works well with standard containers." I
wasn't advocating Josuttis' smart pointer, I was advocating his
examples of how to use smart pointers with the standard containers,
which is what the OP was seeking advice on. Sorry if you
misunderstood.

Best regards,

Tom

 

[Earl Purple]

You're tearing down a straw man. The intrusive smart pointer in the
FAQ does not not involve any inheritance, let alone "all your classes
inheriting from one class." I described it as "perfectly adequate,"
and it is, at least for non-expert users trying to learn about smart
pointers.

Let me describe why I don't think it is "perfectly adequate". Yes, it
will probably help you avoid leaks but breaks some other fundamental
rules. I'll ignore the possible implementation of operator=() using
swap and I'll assume Fred's constructor never throws.

======== the C++ FAQ writes:

class FredPtr;

class Fred {
public:
Fred() : count_(0) /*...*/ { } // All ctors set count_ to 0 !
...
private:
friend class FredPtr; // A friend class
unsigned count_;
// count_ must be initialized to 0 by all constructors
// count_ is the number of FredPtr objects that point at this
};

class FredPtr {
public:
Fred* operator-> () { return p_; }
Fred& operator* () { return *p_; }
FredPtr(Fred* p) : p_(p) { ++p_->count_; } // p must not be NULL
~FredPtr() { if (--p_->count_ == 0) delete p_; }
FredPtr(const FredPtr& p) : p_(p.p_) { ++p_->count_; }
FredPtr& operator= (const FredPtr& p)
{ // DO NOT CHANGE THE ORDER OF THESE STATEMENTS!
// (This order properly handles self-assignment)
// (This order also properly handles recursion, e.g., if a
Fred contains FredPtrs)
Fred* const old = p_;
p_ = p.p_;
++p_->count_;
if (--old->count_ == 0) delete old;
return *this;
}
private:
Fred* p_; // p_ is never NULL
};
==============================

Now I gave 3 rules. This breaks all 2 of them even as it is.It will
break all 3 if we ever decide to make it a template.

(1) If I'm allowed to separate out the implementation of FredPtr from
its interface then I could have a FredPtr with Fred being incomplete.
Change all the above into a template though (assuming that any class
that uses it has to have an accessible count_, either public or by
making the template its friend). (I'm assuming we don't have export).
That's where the option of having Fred derive from a class (that
provides the count) come in as we could get away with only having that
class complete except that we can't forwardly declare that Fred derives
from it so it would probably require a hack with casting, unless anyone
knows of a better way (other than using non-intrusive reference
counting, which is the solution).

(2) Above implementation calls delete. No option for custom deleter.

(3). There is no equivalent for a "const Fred", i.e. a FredConstPtr or
whatever you might want to call it.

Of course there is an alternative to template - you can use
auto-code-generators. In fact these are often a viable alternative, for
example on embedded systems where templates are not permitted.

I fear you may have missed my point. The OP asked for advice about
"the best way to make regular use of smart pointers." I responded that
Josuttis' book - still the best treatise on the Standard Library - "has
some good examples of using a non-intrusive smart pointer (using code
supplied in the book) that works well with standard containers." I
wasn't advocating Josuttis' smart pointer, I was advocating his
examples of how to use smart pointers with the standard containers,
which is what the OP was seeking advice on. Sorry if you
misunderstood.

I will get my Josuttis out at some point when I'm home and have a look.


It may be good if the FAQ were to a "light" version of shared_ptr for
those that can't use boost for whatever reason. Such a version could be
copied and pasted from one file.

I actually think it would be advantageous for boost themselves to have
a "light" version of some of their libraries (i.e. one file no
dependencies).

Of course, even then you might have a problem relating to any copyright
statements at the top of files - will the company allow you to state
that you got the source from "open-source" (i.e. that the company
themselves do not actually own the copyrigth to this particular file).

 

[Earl Purple]

The C++ Standard library doesn't 'new' anything what has to be deleted
by the user and, vice versa, doesn't delete anything that has been
'newed' by the user (similar in C with malloc/free). The only
exception is auto_ptr (and shared_ptr in the future) which violates
that principle.

shared_ptr doesn't call delete on your class, it invokes your deleter.
But as a short-hand, they provided a default deleter for you in case
you didn't provide one. That default one calls delete on your class.

The purpose is not to prevent you from being able to do stupid things,
the purpose is to make it easier for you to write robust code.

If you want transfer-ownership in that you want to be able take the
control away from the shared_ptr, that can be done using the custom
deleter.

The problem, which is not a problem of shared_ptr but a problem of its
use, is that it is often overused, i.e. it is used in a scope where you
know that the lifetime of the object will exist, eg a functor written
like this on a collection of shared pointers:

return_type operator() ( shared_ptr< T > ) /*const */
{
// do something and return whatever
}

of course you know here that there is no need to increase the reference
count on this shared pointer because the collection holding them will
still be there as long as your pointer to it is so

return_type operator() ( const shared_ptr< T > & t_ptr) /* const */;
{
T & t = *t_ptr;
// do stuff with t. If you're doing just one call on it no need for
the above.
}

avoids an unnecessary copy of the shared_ptr (which isn't trivially
cheap, and would be done for every item in your collection).

You can, in the above, potentially make the function above a template
function on its argument. Thus:

template < typename T_PTR > operator() ( T_PTR t_ptr );

etc. This now decouples the functor from the type of pointer your
collection is storing.

Still, you have to understand the developers out there. They are not
always bright enough to understand all of this and I'd rather see a few
unnecessary copies of shared_ptr in code I might have to maintain than
raw pointers everywhere. Optimise later where necessary.

 

[Thomas Tutone]

Let me describe why I don't think it is "perfectly adequate". Yes, it
will probably help you avoid leaks but breaks some other fundamental
rules. I'll ignore the possible implementation of operator=() using
swap and I'll assume Fred's constructor never throws.

[code snipped]

Now I gave 3 rules. This breaks all 2 of them even as it is.It will
break all 3 if we ever decide to make it a template.

(1) If I'm allowed to separate out the implementation of FredPtr from
its interface then I could have a FredPtr with Fred being incomplete.
Change all the above into a template though (assuming that any class
that uses it has to have an accessible count_, either public or by
making the template its friend). (I'm assuming we don't have export).
That's where the option of having Fred derive from a class (that
provides the count) come in as we could get away with only having that
class complete except that we can't forwardly declare that Fred derives
from it so it would probably require a hack with casting, unless anyone
knows of a better way (other than using non-intrusive reference
counting, which is the solution).

(2) Above implementation calls delete. No option for custom deleter.

(3). There is no equivalent for a "const Fred", i.e. a FredConstPtr or
whatever you might want to call it.

Of course there is an alternative to template - you can use
auto-code-generators. In fact these are often a viable alternative, for
example on embedded systems where templates are not permitted.

Let's cut to the chase here. You make some reasonable arguments here
that the FAQ intrusive smart pointer is a naive implementation and is
not perfect.

Here is my response. The relevant comparison for this discussion is
not to a perfect smart pointer. The comparison is to raw pointers.
The OP said that he doesn't use smart pointers, and a fair amount of
bandwidth was spent by various posters on how they don't "trust" smart
pointers.

I referred to the FAQ, which includes straight-forward,
easy-to-understand code. You reasonably point out various flaws in
that code. But if the choices are a naive smart pointer or a naive use
of raw pointers, I'm still going with the smart pointer. And the
advantage of the naive implementation is that a relatively
inexperienced C++ programmer can look at the code, understand it, and
learn from it. He or she may even be more comfortable using such code
than a black-box smart pointer that meets every requirement you place
on it.

So at the end of the day - for production code, I acknowledge your
points. For learning the wisdom of using smart pointers, I hope you
will at least consider - if not acknowledge - mine.

Best regards,

Tom

 

[Earl Purple]

Let's cut to the chase here. You make some reasonable arguments here
that the FAQ intrusive smart pointer is a naive implementation and is
not perfect.

Here is my response. The relevant comparison for this discussion is
not to a perfect smart pointer. The comparison is to raw pointers.
The OP said that he doesn't use smart pointers, and a fair amount of
bandwidth was spent by various posters on how they don't "trust" smart
pointers.
I referred to the FAQ, which includes straight-forward,
easy-to-understand code. You reasonably point out various flaws in
that code. But if the choices are a naive smart pointer or a naive use
of raw pointers, I'm still going with the smart pointer. And the
advantage of the naive implementation is that a relatively
inexperienced C++ programmer can look at the code, understand it, and
learn from it. He or she may even be more comfortable using such code
than a black-box smart pointer that meets every requirement you place
on it.

I would agree that, if you separated the implemention from the
interface in that class and write a swap function and implemented
operator= in terms of it then the class is usable, and might even have
some uses. One, for example, being if you are writing a library that
has a function that returns a newly created "Fred" and that you wanted
to return it as a reference-counted pointer. True that the class is not
a template but you might write a code-generator if you have many
classes like this and if it generated such code with the two changes
I've suggested, it would be reasonable enough in production code (might
not be thread-safe but I guess you could write a special version for
when that is required).

In fact I would rather 3rd party library providers did provide their
own smart pointers than either forcing me to install boost or loki or
whatever or even worse, telling me I have to delete something or call
some method or other when I've finished with something. Internally they
can implement their libraries using boost and I hope they would because
that would make their libraries less likely to leak.

So at the end of the day - for production code, I acknowledge your
points. For learning the wisdom of using smart pointers, I hope you
will at least consider - if not acknowledge - mine.

Not sure what code is written for if not as production code.

My point about that smart-pointer being in the FAQ is that many,
including myself, have treated that FAQ pretty much as a coding
standard guideline, and how could I tell another developer who started
using that smart-pointer above in code that it is not really fit for
use for a couple of reasons I have mentioned when they are likely to
answer that it must be good because it's in the official FAQ.