Mistakes to avoid with C++ 11 smart pointers

http://www.acodersjourney.com/2016/05/top-10-dumb-mistakes-avoid-c-11-smart-pointers/

by debh • 10 years ago

134 90 10 years ago

corysama4• 10 years ago

Not mentioned is passing shared_ptrs as parameters by value. People do it all the time because "passing pointer parameters is as cheap as it gets, right?" But, shared_ptr's thread safety means that each copy involves a full trip past all cache levels to actual DRAM. 100+ cycles each. They are not pointers. They are small yet heavy objects and should be passed by reference. Copying implies assuming shared ownership and should not be done lightly.

Also, "if(!p->expired()) p->lock()->foo()" is not thread safe. The last shared_ptr could die on another thread between the test and the lock. Instead, what you want is "if (auto q = p->lock()) q->foo()"

sneha87• 10 years ago

Interesting point about shared_ptr. Do you think shared_ptrs should be passed by const reference or just reference ?

corysama4• 10 years ago

I'm a "const all the things!" coder in general. If you see something that's not const, that is documentation that you should expect your object to be modified, not that I was too lazy to type 'const' ;P

sharth• 10 years ago

If you're going to pass it by const_ref, why not just pass the actual stored object by const_ref instead?

optforfon• 10 years ago

Typically you should pass raw pointers (constness depends on your use case...). I can't really think of a scenario where the method/function needs to be coupled with the pointer type.

Kristine19753• 10 years ago

Unless they are modified: const reference. Otherwise you cannot pass a temporary object as in:

  void foo(const std::shared_ptr<int>& p);

  ...

  foo(std::make_shared<int>(23));

This will not compile with

  void foo(std::shared_ptr<int>& p);

since a temporary object cannot be implicitly converted to a non-const reference, only to a const one.

pveierland• 10 years ago

Herb Sutter's GotW #91 talks about smart pointer parameters in detail and when to use const and reference:

https://herbsutter.com/2013/06/05/gotw-91-solution-smart-poi...

jstarks• 10 years ago

Atomic memory accesses do not require touching DRAM in the general case. The cost of an atomic operation on a modern Intel CPU is closer to 20 cycles than 100. You will see higher costs if you have two processors trying to access the same shared object concurrently, of course.

Your point is still valid, though -- a shared_ptr should be copied only when you actually want to increase the reference count.

In general, I try to avoid shared_ptr unless I really need reference counting. unique_ptr is much easier to reason about and is much more efficient.

hellofunk3• 10 years ago

I agree, and I frequently use unique_ptr only, and then pass naked pointers to non-owning objects who need a reference. In this way, the code is clear: if it is a regular pointer, it is non-owning always.

Some might argue that using shared pointers is better for this application, but then you must also deal with potential reference cycles and add weak pointers to the mix eventually, and in addition to these concerns, handle any runtime hits for the reference counting.

sytelus• 10 years ago

I generally prefer to pass by const ref. If function needs to make copy, it can always invoke copy constructor. If it doesn't then all is well and performant. I know there are optimizations that can happen if you pass by value and function intends to make a copy. But those are micro optimizations and I've always felt its not worth the confusion it creates to the caller as well as callee. Does anyone else follow this general rule of thumb?

maxlybbert• 10 years ago

I used to do the same thing, until I saw slides ( https://stlab.adobe.com/wiki/images/8/85/2008_06_26_classes_... ) for a talk ( https://my.adobeconnect.com/p53888531/?launcher=false&fcsCon... ) that pointed out that explicitly making the copy makes it much harder for the compiler to do copy elision.

swapna_1956• 10 years ago

So if I'm coding up a LinkedList or a Binary Tree today, instead of using a raw pointer or shared_ptr, should i be using a unique_ptr to prevent circular references ?

debh4• 10 years ago

I think for self referencial data structures, passing them by a const& unique_ptr is a reasonable way to do it.

Thanks, Deb.

solipsism• 10 years ago

I think this is not right -- since references aren't reseatable, this would effectively make the data structure (but not necessarily its contents) immutable.

Shared pointers or raw pointers are the options that make most sense, in my opinion.

Edit: forgot to say, if you use shared_ptr beware circular references. e.g. in the case of a doubly linked list. weak_ptr could be used to break the cycle, but I'd probably use raw pointers here.

n00b101• 10 years ago

For a doubly linked list, I think the right approach is to store a unique_ptr pointing in one direction and a raw pointer pointing in the other direction. It is then understood by convention that raw pointers should never be deleted.

leni5363• 10 years ago

shared_ptr forward, weak_ptr back.

zvrba5• 10 years ago

You should be using a raw pointer, period. The list/tree destructor must be responsible for traversing and deleting the whole structure.

If you have a linked list of, say, 10M elements linked by unique_ptr, deleting the head of the list would cause recursive [1] destruction of all elements in the list. Stack overflow, CRASH, BOOM, BANG!

[1] Take a simple example of list A ->a B ->b C ->c 0 where arrows are unique_ptrs owned by the nodes. Deleting A must invoke the destructor for ->a , which will delete B, invoking the destructor for ->b, which will delete C invoking the destructor for ->c which is null, thus finishing the recursion.

EDITS: added labels to pointers for clarity.

jupp0r2• 10 years ago

You could (and imho should) use RAII semantics using unique_prts for this. Writing custom destructors will introduce unnecessary code that you'll have to maintain and unnecessary bugs that will creep in eventually.

Animats2• 10 years ago

This is why smart pointers via templates don't work well. The mold always leaks through the wallpaper. There's no analysis at compile time to catch those errors.

This is what Rust is for. You really need a borrow checker.

millstone4• 10 years ago

Let me try to identify which ones Rust catches:

"#1: Using a shared pointer where an unique pointer suffices."

The Rust analog would be using Rc or Arc when Box is enough. Rust has the same problem.

"#2: Not making resources/objects shared by shared_ptr thread safe"

Rust does NOT have this problem! You have to be explicit about which objects are thread safe (via Sync), and Rust will not allow you to transfer objects across threads without it.

"#3 : Using auto_ptr"

Naturally, Rust does not have this issue, which is strictly legacy C++.

"#4 Not using make_shared to initialize a shared_ptr !"

"#5 Not assigning an object(raw pointer) to a shared_ptr as soon as it is created"

"#6 Deleting the raw pointer used by the shared_ptr"

These are the same issue: in C++ you can construct a shared_ptr from an object you allocated with new; the reference count must then be allocated separately. Rust does not allow this, and avoids this issue, at the cost of some flexibility (like custom deallocators).

"#7 Not using a custom deleter when using an array of pointers with a shared_ptr"

AFAIK Rust doesn't have this issue, because it doesn't have C++'s array-decays-to-pointer semantics. In Rust the size of the array is part of the type; to be fair, this loses a bit of flexibility, but C++'s semantics here are insane, so point goes to Rust.

"#8 Not avoiding cyclic references when using shared pointers !"

Rust has this problem!

"#9 Not deleting a raw pointer returned by unique_ptr.release() "

Rust has an analog in Box::into_raw(), which is the same issue.

"#10: Not using a expiry check when calling weak_ptr.lock()"

Close one: Rust has the same issue, in that you can call rc::Weak::upgrade().unwrap(). unwrap() is often used with some impunity, especially with locks. However you do have to be explicit about your use of it, while in C++ it's easier to forget. Let's give this one to Rust.

Let me give one more point to C++:

"#11: Accessing an object through a Rc/Arc/RefCell while a caller holds a mutable reference."

This is strictly a Rust problem: because there cannot be more than one extant mutable reference to an object, you have to be careful to not call out to something while holding a mutable reference. This is a doozy.

Total score: There are 7 unique to C++, 1 unique to Rust, and 3 that are shared.

Note that none of this has to do with the borrow checker! Indeed, the whole point of reference types like Arc/Rc/RefCell are when your lifetimes are dynamic, and you need to avoid the borrow checker, which assumes a statically knowable lifetime.

Animats2• 10 years ago

The borrow checker is what makes unique pointers in Rust unique. In C++, you can pass ownership but still reference the unique pointer you moved from. It should crash at run time, deferencing null, but the compiler will happily compile it.

Can you actually cause #11 in Rust? Example?

Circular references remain a problem, but they can only cause memory leaks, not security errors or crashes. It would be nice to have a Rust static analysis tool for circular references. Often, you can prove their absence for a type.

pcwalton• 10 years ago

Your points are technically correct, but I thought I'd add that Rust offers mitigations for these issues in practice:

1. Using shared ptr where unique ptr would do: This happens a lot less in Rust because (a) in C++, shared ptr predated unique ptr, so you see a lot of legacy code needlessly using shared ownership; (b) Rc/Arc prevents mutation without jumping through hoops (RefCell/Mutex), discouraging its use for ergonomic reasons. Additionally, even when it does happen the reference counts are non-atomic on Rc and have to be twiddled manually, so the overhead is much less than with copy constructors which can be invoked silently and often.

2. Reference cycles: This is a real problem. It's somewhat mitigated by the fact that you need a RefCell or Mutex to construct the cycle. If those are absent, no cycle.

3. Box::into_raw: Well, sure, but this method is really rarely used, due to the lessened need for legacy interop. It probably wouldn't make a list of the top 100 mistakes made with smart pointers in Rust, much less the top 10.

4. Double dynamic borrows: This does happen, but you need RefCell or Mutex to do it. It's not really a smart pointer issue at all. In any case, it's there to enforce memory safety, which C++ doesn't even try to; it's not like C++ does this better than Rust.

sseagull• 10 years ago

Two minor comments:

1.) Mistake #7 Has been somewhat fixed in the TS by specializing shared_ptr for array types (similar to unique_ptr<type[]> syntax). What they said was true though for C++11.

2.) If the data within a shared_ptr is not owned by any particular object but is otherwise constant, use shared_ptr<const type>. They can be created from a shared_ptr<type> object. This helps with the aliasing problem mentioned in mistake #1

And a slight nitpick on #4 - I imagine the performance of make_shared depends on the library rather than the compiler itself (and I imagine the performance gain is minimal - it is usually used for readability). Also, C++14 add make_unique, which was overlooked in C++11.

mgraczyk• 10 years ago

make_shared allows the shared pointer to be constructed with its memory and metadata in a contiguous block. This improves cache efficiency and locality and mitigates heap fragmentation. Although make_unique primarily serves to improve readability, make_shared can reduce smart pointer performance overhead dramatically.

See http://stackoverflow.com/questions/24779929/what-happens-whe...

You are right in pointing out that the library determines whether using make_shared will improve performance.

amluto• 10 years ago

The little social media bar, when it pops up (which appears to happen at random) unavoidably obscures the left side of the text when reading in landscape mode. Please consider removing it.

Also, please read up on scroll jank and fix the javascript event handlers (or remove them -- why are you handling scroll events at all?).

debh4• 10 years ago

Thanks for reporting this. Someone else noticed it on a nexus tablet as well. I'm looking into getting this fixed.

yablak• 10 years ago

The first mistake is using shared_ptr to begin with. It's often unnecessary (as the article says, one can often get away with using unique_ptr). When a unique_ptr is not sufficient, a shared_ptr often obfuscates the true owner of the object.

Every object should have one true owner that manages its memory. Anything else will lead to performance regressions and outright bugs.

wmu2• 10 years ago

The first mistake is using shared_ptr to begin with.

Yes, definitively! Using shared pointers can hide bad design.

There are some cases when it is hard to choose the owner of data. Two examples come to my mind. The first are directed graphs: what owns nodes? The second example are parsers, sometimes AST subgraphs must be assigned to extra structures during further processing (type analysis, code generation and so on).

userbinator2• 10 years ago

A rule of thumb that I use is, if you cannot see the owner, then you're probably focusing too narrowly. For a directed graph, which is a "sea of nodes" with no real ordering, I would have the "graph" object own all its nodes; for an AST, if it is truly a tree, then the parent owns its children, or otherwise see the directed graph case.

criddell• 10 years ago

I've been pushed into using shared_ptr when I want to be able to put the pointers into vector (and other containers).

Const-me2• 10 years ago

Disagree.

Last time I used shared_ptr in my code was for a 3D authoring app. Users can load/copy/paste 3D objects, I want a single copy on RAM and VRAM for clones of the same 3D object. If you ask me “why” — because VRAM is limited, and because PCIe bandwidth is a bottleneck.

I‘ve decided that scene node is the true owner of the mesh, and placed shared_ptr<mesh> in my scene node class.

Technically, I could instead decide that scene (i.e. “the graph”) is the true owner of all the meshes in all graph nodes, as you’re saying. Two problems:

1. I would write my own code for memory management and reference counting, for no good reason replacing a perfectly good standard library implementation.

2. Scene is not one true owner that manages the memory of the meshes because of different lifetimes. Users can add/remove stuff from the scene. When they do, both scene nodes and meshes are allocated/deallocated. When two objects are allocated + deallocated together, it’s often a telltale sign that one of them is the true owner of another one. The scene itself lives much longer.

lisper• 10 years ago

Or you could just use lisp and not have to worry about any of this stuff ;-)

dave2000• 10 years ago

And forget about developing games and drivers and OSes and...

pjmlp3• 10 years ago

Once upon a time Lisp was used to write OSes and games....

leni5363• 10 years ago

#11: Do not create two shared_ptr from the same raw ptr! It creates two control blocks and reference counts.

microtonal• 10 years ago

That's #5.

leni5363• 10 years ago

Oh, I missed that one.

Kristine19753• 10 years ago

Mistake #10 contains a race condition when another thread destroys the last shared_ptr to pIceman (Top Gun fan much?) between the call to !pMaverick->myWingMan.expired() and pMaverick->myWingMan.lock().

Just use:

  if (auto p = pMaverick->myWingMan.lock())
    p->take_shower_with(pMaverick);

See http://en.cppreference.com/w/cpp/memory/weak_ptr/lock

userbinator2• 10 years ago

As someone who uses more C than C++, I have often wondered about the value of "smart" pointers and other pseudo-automatic memory management abstractions, since in any case one has to understand the traditional manual memory management to use them correctly, and if they do, they then also have to understand the additional semantics of the class implementing it too --- or bugs like the ones shown in this article will result.

zvrba5• 10 years ago

Most important advantage: RAII. You cannot forget to free the pointee even in the presence of exceptions. The "bugs" described in the article are just a case of not having read the manual. (Like, who in their right mind would assume that wrapping something in a shared_ptr would make it thread-safe?!)

jb1991• 10 years ago

Unique_ptr offers many advantages:

1) Exception safety: what if you throw between your new and delete? Unique_ptr will clean up for you anyway and leave you in a stable state.

2) What if you forget to delete or your delete code never runs? Any of the many problems associated with explicitly writing delete can be non-issues with unique_ptr.

3) Clarity. When you see regular pointers in C or C++, it is never immediately clear who owns them. With unique_ptr, you don't have this problem because it doesn't look like a pointer. If you therefore see pointers, you know immediately they are non-owning.

4) Convenience functions like reset() on unique_ptr handle the release of the old and allocation of the new simultaneously. In general, the less code you have to write is often the better path to safety and maintainability.

5) Unique_ptr handle resources other than memory. It can be used to handle file streams, DB connections, etc and automatically close those also, if you provide a custom deleter function (which can just be a lambda, for elegance).

6) Unique_ptr is not copyable. This is important: if you own resources, it force you to consider this when you copy objects, and the compiler will force you to write your own copy constructor. Without this, it is easy to get into trouble if you forget these things with raw pointers.

7) Many syntactic improvements: i.e. unique_ptr will automatically call delete or delete[] based on what needs to happen, you can't accidentally get it wrong.

This is just a start list. The value of unique_ptr is considerable.

random_upvoter• 10 years ago

I totally agree. In most software projects, memory management isn't a particularly difficult aspect of the problem. With todays software tools, if your code contains memory leaks or pointer bugs you're just being an amateur. And in those cases where memory management requires a bit more thought and design it's rather unlikely that some standard smart pointer protocol is going to be a glove-fit solution. So I never saw the point of smart pointers unless you want to reinvent Python or something.

pjmlp3• 10 years ago

I develop software since the mid-80s.

Never been part of a C or C++ project at the enterprise level, where any single developer could answer any question about memory management tracking.

Tracking down crashes due to memory mismanagement was always a several days task.

jasonjei• 10 years ago

If I remember correctly, C++11 smart pointers need more maintenance when being passed on a message pump across threads, particularly from my memory of using it on Windows.[0]

When sending/posting a smart pointer to a different thread's message pump via a function like PostThreadMessage, the sending thread's smart pointer would go out of scope and the memory would be released because the receiving thread hadn't increased the reference count. By the time the receiving thread's message pump processed it, the pointer had already gone out of scope.

If you are using the producer-consumer pattern, most likely you'll have to use unique pointers and call release manually; I guess that's still better than using new and delete, right? Or perhaps I could try using SendMessage instead of PostMessage and read the return value to determine if the pointer got to the other side (for some reason, that wasn't an option to me).

[0] http://stackoverflow.com/questions/11290912/passing-objects-...

gpderetta2• 10 years ago

I assume that postthreadmessage has a C abstraction. In C++ you should use a queue that properly supports copyable and movable types. Or at the very least write a wrapper on top of postthreadmessage that does.

zvrba5• 10 years ago

Mistake #2 is just bizzare: who in their right mind could assume that wrapping an object in shared_ptr makes it threadsafe?! And the mistake is worded as if you should make these object thread-safe, which is just as bizzare.

jupp0r2• 10 years ago

Mistake #0:

Use smart ptrs (especially shared_ptrs) instead of having clear memory ownership contracts and incorporating memory ownership into your design. Two objects each having a shared_ptr to the same object as a member should raise red flags.

hellofunk3• 10 years ago

instead of having clear memory ownership contracts

You have a good point for shared_ptr, but unique_ptr offers the best clarity for ownership, much better than raw pointers, since owning and non-owning pointers all look the same, but a unique_ptr that hands out regular pointers to its memory makes it clear in the code who owns what.

inlined• 10 years ago

Mistake 10 has a race condition. The shared pointer could be cleaned up between the weak pointer expiry check and lock. Why not just lock and check for null before using it?

superbatfish• 10 years ago

You're right. These were all good tips except for #10, where the "solution" is not thread-safe. Checking for null is better.

moonshinefe2• 10 years ago

Ah, so C++ pointers, even with "smart pointers" is still complicated and massively error prone.

I thought that's sort of what they were meant to fix. Apologies, haven't coded in C++ for several years (when auto_ptr still roamed the Earth). But it doesn't inspire confidence reading things like this that we've made much progress...

tux3• 10 years ago

I might be biased, but I don't know that you can really call it massively error prone. Certainly, if you want to break the rules C++ will assume you have a good reason for it and let you shoot you in the foot all the way through the Earth's crust, but the smart pointer rules are actually fairly simple this time.

For example take the first mistake: "using a shared pointer where an unique pointer suffices". Unique pointers and shared pointers mean completely different things, if you know what they are, you would never think of mixing one for the other.

If I had to make an analogy, this is like "using a video with a single frame when an image suffices". The video may work, but that's not at all how you're supposed to use it, and surely, anyone who knows what a video and an image are would not make that mistake!

You'll notice that most of the remaining mistakes here are with the shared pointer and weak pointer. Thankfully, in the overwhelming majority of cases you don't need it at all, sticking to the very simple unique_ptr is what you want, and unique_ptr is practically FootProof™.

moonshinefe2• 10 years ago

I see your point of view, and I agree that in the right programmer's hands, it's fine. Unfortunately, as we've seen repeatedly, people over estimate their pointer abilities and it leads to all sorts of memory and security issues.

I figure the C++ smart pointers should be as easy to use as possible, without caveats vs. C style pointers or whatever.

Maybe that isn't practical in the C++ language design right now, but that's just how I see it.

cheers

mike_hock• 10 years ago

They're as "massively error-prone" as garbage-collected references in other languages.

#1

Performance aside, unique_ptr gives you something that most languages don't have, unique ownership enforced at the language level. This makes code less error-prone. Disregarding #1 just gives you what you get everywhere else.

#2

Basically just says, if your object isn't thread-safe, don't use it concurrently. Again, nothing specific to C++.

#3, #4, #5, #6, #7

Well duh, don't use manual allocation or the deprecated auto_ptr in the first place.

#8

That's the only real gotcha, and the reason why weak_ptr exists. It's the price for having RAII and destructors (because when the destructor is called, all your class members still have to be alive).

In return for keeping your pointer graph cycle-free, you get actual automatic resource management that isn't limited to memory.

blubb-fish• 10 years ago

mistake #1: not using Rust instead!

(I learned that on HackerNews - didn't write a single line of C++ or Rust in my life, though)

koshak• 10 years ago

http://harmful.cat-v.org/software/c++/linus

once-in-a-while• 10 years ago

Shudder, shudder, C++ 11 seems as bad a monster as C++ always has been...

nokeya• 10 years ago

Why everyone hating new/delete so much? Instead of this headache with smart pointers and all complicated stuff they bring, clear and easily understandable memory management.

zanny3• 10 years ago

Because as a computer scientist the last thing you should ever want to do is to do something manually. Because when you do something by hand, every time - especially something as simple as heap allocated objects - you will do it wrong all the time. And memory management is complicated, even if the primitives are not. So when you combine something one level of indirection away complex with a ton of boilerplate you get constant problems, which 25 years of C++ have demonstrated repeatedly in the context of raw memory operators.

These smart pointers, in reality, are not a headache. You want to make a heap object? make_unique. You want to pass it around? Go for it. You want to pass ownership? std::move it. And only in the extremely rare case where you have multiple objects of non-deterministic lifetime that need to both access the exact same data structure, then you pull out shared_ptr and deal with the synchronization.

But unique_ptr, by itself, is probably the most productive addition to C++ period. More than the STL, more than templates, if you use it correctly and design your code properly, it can replace every heap allocation and drastically reduce code complexity.

makecheck• 10 years ago

The biggest mistake with these objects is over-using them. They are very helpful for internal memory management but they should definitely be avoided in APIs unless the point of the API is to do something like transfer ownership (e.g. factory method returning std::unique_ptr<T>).

Raw pointers continue to be fine in a wide variety of cases. Suppose the object model has something like a type T with APIs to access pointers to types A, B and C, the lifetimes of A, B and C are always tied logically to the lifetime of T and everything you do with the pointers to A, B and C is in the context of some T. This does not require special magic pointer objects to “share” A, B and C pointers because their safety is ensured transitively. So you might wrap T but you don’t need to wrap the others.

Besides, pointer objects are still annoying to use: sometimes they’re pointer-like (e.g. "if (somePtr)" works) and sometimes they’re not (requiring a ".get()"), and they seem to “infect” entire call chains. If you have previous API dependencies on boost::shared_ptr<>, you can’t easily replace those with std::shared_ptr<> either. Use them very judiciously, and always try the simplest thing that would work, first.

debh4• 10 years ago

Good points ! I ran into some difficulty translating the boost::shared_ptrs to C++ 11 standard shared_ptrs a while back while adopting some open source tech for xbox.

Also you're spot on about people touting the smart pointers as a silver bullet for all memory issues leading to an overuse of these otherwise fantastic tools.

TwoBit• 10 years ago

I'm not completely understanding the recommendation for #5:

Recommendation: If you’re not using make_shared to create the shared_ptr , at least create the object managed by the smart pointer in the same line of code – like :

shared_ptr<aircraft> pAircraft(new Aircraft("F-16")); </aircraft>

I think he really means to say that the second shared_ptr is better initialized from the first shared_ptr instead of the raw ptr.

Const-me2• 10 years ago

His last fix is buggy:

    if( !pMaverick->myWingMan.expired() )
    {
        cout << pMaverick->myWingMan.lock()->m_flyCount << endl;
    }

The problem here is it will still crash if another thread destroys the object between line #1 and line #3.

Here’a good fix, from http://en.cppreference.com/w/cpp/memory/weak_ptr/lock

    if( auto observe = weak.lock() ) {
        std::cout << "\tobserve() able to lock weak_ptr<>, value=" << *observe << "\n";
    }
    else {
        std::cout << "\tobserve() unable to lock weak_ptr<>\n";
    }

debh4• 10 years ago

Yep - thanks for calling this out. I should have put out a version that's safe in multithreaded environment. I've updated the article to reflect this.

abhinav210• 10 years ago

Can anyone point to a similar article for using C++ casts?

Mistakes to avoid with C++ 11 smart pointers

Related Stories