Sign in to follow this  
Nanook

Overusing smart pointers?

Recommended Posts

Nanook    537
I have a feeling that I might be overusing smart pointers.. Does anyone know of a good article on when to use them and when not to?

Share this post


Link to post
Share on other sites
Rene Z    605
Probably not.

I always use smart pointers, unless there's a good reason not to. A good reason can be performance in tight loops or something like that, but those cases are rare and should never leak out of your interfaces. Note that using smart pointers doesn't mean 'don't think about ownership', which often results in overusing shared pointers. Quite the opposite in fact: different types of smart pointers are a great way to explicitly state ownership of an object.

Share this post


Link to post
Share on other sites
kunos    2254
Actually Stoustrup himself suggests to use smart pointers as "last resort", expecially shared_ptr . The danger of introducing smart pointers as standard in C++ is, IMO, the urge that people have to use them everywhere, and I believe that is wrong for a series of reasons but the top one is: they really make C++ uglier than already is, having this const shared_ptr<Node>& all over the place is really ugly.

C++ object lifetime should be managed with the following priority:

1) Stack based. This is the way C++ was designed for. If you can make it stack based, do it. Cleanest and fastest code is obtained this way.
2) Clean ownership heap based with new and delete. One guy creates the object, same guy is responsible to delete it.
3) unique_ptr .. which is like 2 but if you are willing to write unique_ptr<blabla>, .get() and all that nonsense over and over again to AVOID 1 SINGLE CALL TO "delete" in your destructor.
4) shared_ptr . That is, when the ownership is _really_ SHARED, so you can't possibly predict the lifetime and ownership of an object and this is used and kept alive by different objects.

So while I can't possibly think about using option 3 ever.. I know there are scenarios where 4 is useful. I use shared_ptr for materials for example, and it works ok.
Also, shared_ptr can create leaks that are REALLY hard to detect if you manage to create a reference loop.

All the above is IMO IMO IMO .. smart pointes just made it into the C++11 standard and there is still no book about best practices out yet... so the jury is still out on this one. Edited by kunos

Share this post


Link to post
Share on other sites
the_edd    2109
[quote name='kunos' timestamp='1339952582' post='4950046']
Actually Stoustrup himself suggests to use smart pointers as "last resort", expecially shared_ptr . The danger of introducing smart pointers as standard in C++ is, IMO, the urge that people have to use them everywhere, and I believe that is wrong for a series of reasons but the top one is: they really make C++ uglier than already is, having this const shared_ptr<Node>& all over the place is really ugly.
[/quote]
IMHO, correctness should come before aesthetics every time. typedefs can be a big help, as can [i]auto [/i]if available.

[quote]
C++ object lifetime should be managed with the following priority:

1) Stack based. This is the way C++ was designed for. If you can make it stack based, do it. Cleanest and fastest code is obtained this way.
[/quote]
I definitely agree with this.

[quote]
2) Clean ownership heap based with new and delete. One guy creates the object, same guy is responsible to delete it.
3) unique_ptr .. which is like 2 but if you are willing to write unique_ptr<blabla>, .get() and all that nonsense over and over again to AVOID 1 SINGLE CALL TO "delete" in your destructor.
[/quote]
If you've got unique_ptr, you've quite possibly got [i]auto [/i]too. You get brevity with additional safety for free. And the more you standardise on smart pointers, the less you need .get().

Besides, without the use of something like unique_ptr, one would typically be required to make more than one call to delete where error handling occurs, early returns, inside surrounding catch clauses, etc.

Share this post


Link to post
Share on other sites
Matias Goldberg    9574
I see a flamewar coming, let's keep it calm.

The problem with auto is that it can also be overused. You get safety and smaller lines of code. But it's also harder to grok for another human being who didn't write the code. You have no idea what the variable type holds unless you make [i][b]more[/b][/i] use of advanced IDE tools. Furthermore I'm starting to see programmers using auto for almost every single variable they declare (being completely unnecessary btw).
Using auto inside a template is incredibly useful because by definition, templates don't know the type of objects that are being passed into them until compiled.
But IMHO, outside of a template, the usage of auto should only be used as a last resort; typically where using auto has a real benefit over explicitly declaring the type of the variable. This isn't an old problem, interpreted languages (Python, Lua) already have this issue. It's not a problem about safety, style, or aesthetics. It's a problem of deeply understanding what your (or someone else's) code is doing, which is very important in close-to-the-metal languages like Asm, C & C++.

In Python & Lua, this problem is alleviated because one can write "print( variable )" and they also have an interactive interpreter. In C++ we have none of that.

Share this post


Link to post
Share on other sites
njpaul    367
I tend to rarely use smart pointers. When I first was introduced to them, I overused shared_ptr, and I would often end up with some interesting crashes in destructors on program exit. Then I started thinking more about why I actually needed them for my situation, and it turns out I didn't. I often have manager classes that are the sole place for creation and destruction of whatever resource they're managing. When I need a pointer to a resource, I ask the manager for it.

Not that there aren't any reasons to use smart pointers, but I tend to structure code in a way where I don't need them. Sometimes this can't be done, and sometimes it'd just be too much work to refactor the code. I know of a multi-million line program that is littered with shared pointers, and occasionally those developers will run into a problem with them.

Share this post


Link to post
Share on other sites
japro    887
shared_ptr can be overused unique_ptr on the other hand has no overhead anyway (except maybe for the syntactical one). But the benefits you get are a way lower chance to produce memory leaks, exception safety etc... Also the "overhead" of writing unique_ptr is often compensated by the fact that you don't even need an explicit constructor on lots of cases.

Anyone who dismisses smart pointers with "all it saves you is a delete in the destructor" has apparently never heard of exception safety.

Share this post


Link to post
Share on other sites
wack    1358
So far I have mostly used unique_ptr (or the old auto_ptr), because it's a simple way of getting exception safety when allocating something locally.

Can't think of any time when I've had to resort to shared_ptr, there always seems to be a better option.

Share this post


Link to post
Share on other sites
alvaro    21246
In my own code I tend to use local variables and standard containers the vast majority of the time, which makes explicit use of pointers rather infrequent. I think this style of programming has saved me from many headaches, and I strongly advocate it.

When I do have to use a pointer (e.g., for the return value of a factory), I often end up using a shared_ptr because I don't trust the caller of that code (usually me) with not messing up the ownership. But I don't feel very strongly about this choice and perhaps I'll change it in the future.

Share this post


Link to post
Share on other sites
SiCrane    11839
For factory functions I prefer to return a auto_ptr or unique_ptr (depending on the compiler support I have to work with) as then that says that this is a new object that no one else has any references to rather than being a potentially shared object. Since shared_ptr has a constructor that takes an auto_ptr or unique_ptr, you can sink it into a shared_ptr if you want to do sharing, but the factory doesn't force sharing overhead on you if you don't want it.

Share this post


Link to post
Share on other sites
krippy2k8    646
I won't too far into the debate over shared pointers except to say that I am a big fan, they are extremely useful in the right circumstances, but as with most programming constructs they can, of course, be overused. So take the time to learn as much as you can about them if you're going to use them.

I will also say that I've found unique_ptr is very handy for creating temporary buffers of dynamic size, or ones of static size that you don't necessarily want eating up your stack.

i.e:

[CODE]std::unique_ptr<char[]> buffer( new char[64 * 1024] );[/CODE]

Then you don't need to worry about exception safety or calling delete in every exit point of your function.

Share this post


Link to post
Share on other sites
Neilo    290
[quote name='kunos' timestamp='1339952582' post='4950046']
3) unique_ptr .. which is like 2 but if you are willing to write unique_ptr<blabla>, .get() and all that nonsense over and over again to AVOID 1 SINGLE CALL TO "delete" in your destructor.
[/quote]

Somewhat agree, somewhat disagree with this point.

I don't use exceptions in my C++, but using a unique or scoped pointer makes your code more exception safe.

Even if you don't use exceptions, for allocations within a function, it take out a lot of the programmer overhead of memory management. Then, for consistency's sake, I use scoped pointer wrapper for member variables too.

I do agree that littering code with myPtr.get() is a bit of a pain in the neck though!

Share this post


Link to post
Share on other sites
japro    887
Even if you are not explicitly using exceptions yourself the standard library still does and new will also throw unless you explicitly use "nothrow"... But then if you are not using exceptions you will probably also not catch those and the program will simply terminate.

Share this post


Link to post
Share on other sites
Slavik81    360
Assuming the pointer must not be null, is there any reason to use pointer.get() over &*pointer? I've been using the latter as it works for pretty much any pointer-like type (raw pointers, boost smart pointers, qt smart pointers).

Share this post


Link to post
Share on other sites
krippy2k8    646
[quote name='Slavik81' timestamp='1340041320' post='4950315']
Assuming the pointer must not be null, is there any reason to use pointer.get() over &*pointer? I've been using the latter as it works for pretty much any pointer-like type (raw pointers, boost smart pointers, qt smart pointers).
[/quote]

From a functional standpoint, no. From a code clarity standpoint, yes. pointer.get() is clear in it's intent, &*pointer looks like a bug.

That being said, if your code is littered with .get() then you may have a more fundamental code smell; overusing pointers, period. You should almost always prefer to pass by const reference rather than by pointer unless you have a good reason to pass a pointer (i.e. an output parameter, a raw buffer, or interfacing to a third-party library). So most access to the object should either be pointer-> or *pointer rather than pointer.get().

Share this post


Link to post
Share on other sites
clickalot    177
The only issue with over-using smart pointers is that they will make you feel safe while coding.
That feeling of safety might lead to a messy flow in your game. Using normal pointers forces you to have a clean flow in your code, a flow of initialization, the ownership for each object is clearly defined, etc.
I've seen this a lot in school projects (university), and to some degree even in some engines used to make games :).
You can end up with some object shared between two or three others, with no clearly defined owner and this might give you a lot of headaches. Edited by clickalot

Share this post


Link to post
Share on other sites
wqking    761
I have some policies that I always stick with in my personal C++ development.

1, Always use clear and explicit ownership.
2, Prefer scoped pointer (unique_ptr) to raw pointer.
3, Avoid shared pointer unless there is good reason. Policy 1 implicits that shared pointer should be avoided. The only good reason to use shared pointer I've found recently is that, when there is no clear ownership of the resource. Such as in a script binding, the C++ resource may be freed by script engine without knowing by your code.

With those policies, I use scoped pointer extensively but I never overuse shared pointer, and I rarely get memory leak or bad pointer.

Just my 2 cents

Share this post


Link to post
Share on other sites
Nanook    537
I've started changing alot of my code now.

But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?

[CODE]
class A
{

};

class B
{
public:
void SetCurrent(A* current) { m_current = current; }
private:
A* m_current;
};

B b;
std::vector<std::unique<A>> vec;
std::unique<A> p0 = CreateA();
std::unique<A> p1 = CreateA();
vec.push_back(std::move(p0));
vec.push_back(std::move(p1))
b.SetCurrent(p1.get());
[/CODE] Edited by Nanook

Share this post


Link to post
Share on other sites
y2kiah    2031
[quote name='Nanook' timestamp='1340111272' post='4950562']
should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?
[/quote]

Don't mix raw pointers with smart pointers, by storing raw pointers you completely negate the safety and validity of your smart pointers. You can .get() the raw pointer for local operations where there is no risk of memory corruption.

In this case you probably want to pass a shared_ptr to A and store it as either a shared_ptr or weak_ptr. If you actually intend to transfer ownership of A into B, then you can pass unique_ptr.

unique_ptr is not as useful when you have a pointer that you need to pass around a lot. unique_ptr is very useful for heap-allocated objects within initialization functions that might fail early, relieving you of a lot of ugly cleanup work. I would also agree that unique_ptr is useful for returning objects from a factory, but not so much if there is a cache involved. If there is a cache, what you actually have is shared ownership (this cache holds onto one reference while another reference is grabbed by the object calling the factory).

Share this post


Link to post
Share on other sites
sundersoft    216
[quote name='Nanook' timestamp='1340111272' post='4950562']
I've started changing alot of my code now.

But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?
[/quote]

Raw pointer. If the object is deleted and then accessed by the currently selected pointer, that would usually indicate an error in your code and both weak_ptr and raw pointers will cause a runtime error here (assuming you don't check for a null shared_ptr from the weak_ptr's lock function). It's possible that a memory access to deleted memory will not cause a runtime error but this is rare and usually yields garbage data.

Using shared_ptr generally implies that there is shared ownership of the pointed-to object, so having only one shared_ptr to an object means that there is exclusive ownership, which makes the code less clear in addition to having more overhead. It's also possible that a second shared_ptr could be created by mistake which points to the same object, which is generally not desired (removing the object from the container should delete it) and not possible with the unique_ptr implementation.

Using shared_ptr for both the currently selected pointer and the container of objects would cause the object to remain in memory when the object is removed from the container while being referenced by the currently selected pointer, but this condition is usually errorneous (the selected pointer should generally only point to objects in the container), so this will mask the error and make it hard to detect.

The object may require shared ownership without considering the existance of the currently selected pointer (for example, if the object is immutable then you might want to copy a pointer to it instead of doing a full copy, in which case shared_ptr is appropriate). In this case, weak_ptr should be preferred since the overhead of having another weak_ptr instead of a raw pointer when the shared_ptr already exists is neglegible and the weak_ptr is more likely to detect errors. However, checking if the shared_ptr returned by weak_ptr's lock function is null should usually be omitted since it is usually a non-recoverable error to have the weak_ptr return a null shared_ptr.

Share this post


Link to post
Share on other sites
wack    1358
This is just my opinion, which may be a little bit on the conservative side, but I haven't seen much point in passing unique_ptrs around or keeping them in containers. This is a short example of what would be be a typical usage of unique_ptr for me:

[code]

unique_ptr<tempfile_c> tempfile(new tempfile_c(ios::binary|ios::out));
if (!tempfile->is_open())
return FATAL_ERROR_TEMPFILE;
//insert other early returncode or code with possible exceptions here
m_binary_tempfiles.insert(make_pair(field_id, tempfile.release()));
[/code]
In the above example, the pointer is owned by the unique_ptr until it's ready to be assigned to the real owner (m_binary_tempfiles, a memer variable) with release().
Sure, I still have to clean m_binary_tempfiles in the destructor, but that's fine by me - it's all exception safe, and the data type declaration of m_binary_tempfiles doesn't make eyes bleed.

[quote name='Nanook' timestamp='1340111272' post='4950562']
I've started changing alot of my code now.

But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?

[/quote]

Share this post


Link to post
Share on other sites
larspensjo    1561
[quote name='Nanook' timestamp='1340111272' post='4950562']
But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?
[/quote]

Obviously, your code works, but the current use of smart pointers is an overhead with no added benefits. If it wouldn't have been for the need of secondary "selected object", then it would seem that vector should have been a vector of unique_ptr instead.

The question is, what is the wanted result of the design for this use case? We want (somewhat overlapping):[list]
[*]Flexibility that allows for change
[*]Decreased chance that a change will lead to dangling pointers
[*]Maintain efficiency
[/list]
[quote name='y2kiah' timestamp='1340128308' post='4950653']
In this case you probably want to pass a shared_ptr to A and store it as either a shared_ptr or weak_ptr.
[/quote]

This would fulfil most requirements, except adding some overhead. That is, use shared_ptr for the vector and weak_ptr for the selected object. If it is not a benchmarked and proven performance critical section, I would not worry about the overhead. Start out with a "fool proof" implementation and worry about optimization later.

Adding smart pointers to source code retroactively is a little like adding "const". It will have rippling effects, spreading around, which can get out-of-hands.

Share this post


Link to post
Share on other sites
Zoomulator    273
[quote name='kunos' timestamp='1339952582' post='4950046']
Actually Stoustrup himself suggests to use smart pointers as "last resort", expecially shared_ptr .
[/quote]
It depends on what you mean with smart pointers. On the Going Native conference earlier this year they discussed the new smart pointers, that is both the unique_ptr and shared_ptr.

B. Stroustrup did say shared_ptr should be a last resort, but the unique_ptr should [i]absolutely[/i] be used instead of any new/delete case. It was clearly stated that this should be the new way of handling heap objects in C++. "You should never have to call delete on your own."

Raw pointers still has its uses though, as also discussed at the conference, but they should always be perceived as non-owning pointers that you must guarantee never outlives the object it points to. This is usually the case when a child object has to point back to its parent, where the parent holds an owning pointer of the child. You could use a weak_ptr here, but it's pointless and creates unnecessary overhead. - The intention of this is however only clear if you always use the new smart pointers in the other cases, which is a small theoretical change in code style, but a very large practical change.

When ever I have a class that I put on heap, I usually put in a public typedef for a unique_ptr of that class. Simply MyClass::uptr_t, which honestly isn't that more straining to write that MyClass*. Or declare it outside the class as MyClass_uptr. The added syntax by unique_ptr<> is a poor excuse not to use them. Who can't say they should use typedef a bit more anyway? Edited by Zoomulator

Share this post


Link to post
Share on other sites
kunos    2254
[quote name='Zoomulator' timestamp='1340184439' post='4950896']
[quote name='kunos' timestamp='1339952582' post='4950046']
Actually Stoustrup himself suggests to use smart pointers as "last resort", expecially shared_ptr .
[/quote]
It depends on what you mean with smart pointers. On the Going Native conference earlier this year they discussed the new smart pointers, that is both the unique_ptr and shared_ptr.

B. Stroustrup did say shared_ptr should be a last resort, but the unique_ptr should [i]absolutely[/i] be used instead of any new/delete case. It was clearly stated that this should be the new way of handling heap objects in C++. "You should never have to call delete on your own."

Raw pointers still has its uses though, as also discussed at the conference, but they should always be perceived as non-owning pointers that you must guarantee never outlives the object it points to. This is usually the case when a child object has to point back to its parent, where the parent holds an owning pointer of the child. You could use a weak_ptr here, but it's pointless and creates unnecessary overhead. - The intention of this is however only clear if you always use the new smart pointers in the other cases, which is a small theoretical change in code style, but a very large practical change.

When ever I have a class that I put on heap, I usually put in a public typedef for a unique_ptr of that class. Simply MyClass::uptr_t, which honestly isn't that more straining to write that MyClass*. Or declare it outside the class as MyClass_uptr. The added syntax by unique_ptr<> is a poor excuse not to use them. Who can't say they should use typedef a bit more anyway?
[/quote]

I agree.. but, to me, they feel too much as an afterthought and totally extraneous to the language. If the committee REALLY feels like unique_ptr should take the place of new/delete why not implement it in the LANGUAGE.. just as Microsoft does with their "Node^" thing in .NET ? Node^ might then be converted to shared_ptr<Node> by the preprocessor.. and come up with something else for unique_ptr .. ResourceManager@ ? That will generate ugly stuff anyway.. const Node^ & node .. but still better than totally arbitrary typedefs. Edited by kunos

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this