Mistakes to avoid with C++ 11 smart pointers
acodersjourney.com
acodersjourney.com
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()"
>> That's a good point. If the function will eventually lead to some object retaining a reference, then you should use a non-const shared_ptr ref. The assignment into the object will do the copy constructor and that needs a non-const ref. But, you don't need to be making temp object copies along the way. If the function will not lead to something retaining a reference, then you shouldn't be passing the reference retention object to it. Just pass a direct ref to the target object.
Hum? What assignment into the object? Where I'd typically see taking a `const std::shared_ptr<> &` to signal "retaining ownership" would be something like this:
class Foo {
public:
void AppendChild(const std::shared_ptr<Foo> &x) {
children_.emplace_back(x);
}
private:
std::vector<std::shared_ptr<Foo>> children_;
};
This also gives callers the flexibility to std::move into your argument, transferring ownership.
A good rule of thumb is, if you are going to unconditionally take ownership of an object, accept it by value.
void foo(const std::shared_ptr<int>& p);
...
foo(std::make_shared<int>(23));
void foo(std::shared_ptr<int>& p);
https://herbsutter.com/2013/06/05/gotw-91-solution-smart-poi...
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.
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.
Every object should have one true owner that manages its memory. Anything else will lead to performance regressions and outright bugs.
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).
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.
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.
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.
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.
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.
Just use:
if (auto p = pMaverick->myWingMan.lock())
p->take_shower_with(pMaverick);
This is what Rust is for. You really need a borrow checker.
"#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.
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.
An example of #11:
use std::cell::RefCell;
fn main() {
let c = RefCell::new(5);
*c.borrow_mut() = *c.borrow() + 1;
}
A real world example of this: say you are keeping a count of some event in a global or thread local storage. You pull out a mutable reference, so you can update the count more efficiently. You then call a function which accesses that global. Crash!
Rc/Arc have the same issue. This defeats the point of refcounting: it means you must have global knowledge of the refcount in order to reliably call mutating functions on the contents. But if you have that global knowledge, you wouldn't need refcounting in the first place!
> but crashes at runtime in Rust
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.
if( !pMaverick->myWingMan.expired() )
{
cout << pMaverick->myWingMan.lock()->m_flyCount << endl;
}
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";
}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.
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.
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-...
Thanks, Deb.
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.
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.
You should of course use smart pointers for the automatic pointers that temporarily hold node references in functions that manipulate the datastructure.
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.
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...
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™.
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
The issues in the article had me banging my head, because they aren't how smart pointers are used in practice. The only time that you would call release(), for example, is when you are passing a pointer into a pre-C++11 API. Once you have done so, you wouldn't be worrying about calling delete anyways, because you have transferred ownership.
So you may design a better pointer for them (if possible) and still have a huge spaghetti ugly bug-ridden insecure unmaintainable codebase... But at least the pointers are easy!
Even of you make them program in a rubber room helmet language, they are going to cause just as many problems.
Tools like unique_ptr are meant to let the good programmers work more expressively and make fewer mistakes. They aren't meant to let random folks write perfect code without thinking and learning first.
> #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.
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.
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.
Also, please read up on scroll jank and fix the javascript event handlers (or remove them -- why are you handling scroll events at all?).
(I learned that on HackerNews - didn't write a single line of C++ or Rust in my life, though)
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.