Hacker News Reader

Comment by jeremysalwen | Hacker News Reader

I disagree with this article. There is a case where you might want to pass a const reference to a shared pointer.

Suppose you have a class

  class Foo {
    shared_ptr<Widget> widget;
    bool has_widget();
    void set_widget(shared_ptr<widget> widget);
  }

and you are writing a function which may call set_widget(), it can save an unnecessary shared_ptr copy if you pass it as a const reference:

  void maybe_set_widget( Foo& foo, const shared_ptr<Widget>& widget) {
    if(!foo.has_widget()) {
       foo.set_widget(widget); 
    }
  }

Doing a quick google search finds that Herb Sutter agrees with me (https://herbsutter.com/2013/06/05/gotw-91-solution-smart-poi...):

>Use a const shared_ptr& as a parameter only if you’re not sure whether or not you’ll take a copy and share ownership

View on HN

This is not untrue, but the counterpoint would be that the intersection of a) the number of situations where the shared pointer copy would have any measurable influence on performance, and b) using shared pointers is a good idea for that particular problem in the first place, is likely vanishingly small.

The main performance ‘problem’ with shared pointers is locking and contention in heavily multithreaded scenarios, which is why pretty much nobody uses a reference counting memory model for these kinds of things. In the vast majority of single or lightly threaded scenarios I think you will be hard pressed to find any reasonable algorithm implemented using shared pointers where the reference counting overhead is even measurable. So it’s not a strange thing at all to deprioritize hypothetical performance gains over clear intent of your code. Best thing is to stay away from shared pointers unless you really need them anyway, as they promote designs with sloppy ownership semantics. Been there, done that, try to get rid of them years later... :-/

The almost obligatory Herb Sutter reference made me chuckle by the way. He says so many things about what he thinks is the right thing to do in C++, and gets quoted almost as an absolute authority on good C++ practices, but quite often (not always, mind you) I find his arguments weak and unconvincing, and his recommendations sometimes downright counterproductive.

> Best thing is to stay away from shared pointers unless you really need them anyway, as they promote designs with sloppy ownership semantics.

Could you elaborate? I'm not an expert in C++ but smart pointers did simplify a lot of my code and I never met the case where it was a problem.

Like everything else in C++ (and any low-level language), they should be used carefully.

In my experience unique pointers are usually a better fit than shared pointers. That is not to say shared pointers do not have their place, but they can definitely be abused. Especially for developers that come from garbage collected languages, it might be tempting to replace all pointers with shared pointers and call it a day. Not only can this cause performance issues when these are used in performance critical locations, but you can also lose track of who owns what very easily.

It is also very easy to accidentally create cyclic references with shared pointers. Suppose you have a class like this:

   struct Node {
      shared_ptr<Node> parent;
      shared_ptr<Node> child;
   };

This rather innocent looking class creates a cycle when you fill in the values, because the parent and child reference each other. That cycle now means your nodes will not be cleaned up anymore.

Unique pointers do not have any of these problems. They cannot create cycles, are very cheap and they make it very explicit who owns what. They are not suitable for every situation (i.e. when there is actually shared ownership shared pointers should be used) but in most situations I find that they are sufficient and have a lot less pitfalls than shared pointers.

> They cannot create cycles,

I don't think this is true. Couldn't you accidentally make two objects "own" each other?

Yes, excellent point. It is possible to make cycles, it is just a lot more difficult to do accidentally than with shared pointers.

> This rather innocent looking class creates a cycle when you fill in the values, because the parent and child reference each other. That cycle now means your nodes will not be cleaned up anymore.

It’s fairly common knowledge, but just to answer the inevitable question: you can avoid the cycle with weak pointers.

With shared pointers it is very hard to reason at which point things get deleted, from just looking at the code. This can become particularly messy when you communicate them freely across API boundaries, because that allows callers to just store the shared pointers they get from your API instead of e.g. making private copies or not keeping references at all. Especially in large code bases worked on by multiple people, object lifetime will grow beyond anyone’s comprehension, and people writing new code on top of the existing code will have little other choice than just default to copying around the shared pointers everywhere.

If you don’t know what the lifetime of your objects is, it is very difficult to follow RAII best practices, to control memory use (a shared pointer that is held on to where it is not necessary is basically a leak), to avoid high and/or unpredictable allocation/deallocation cost (compared to e.g. arena allocators or just stack copies), etc. On top of that there is a very conceivable risk of introducing cycles (and hence leaks) when overusing shared pointers. And last but not least debugging code that defaults to shared pointers everywhere can be a nightmare.

These are all practical downsides, but to me personally the argument that you have to be explicit about your memory model (allocation, ownership, lifetime) and use the right tools for your design is even more important. Shared pointers are definitely a very useful tool, but only in situations where you truly want to have shared ownership, and not just because they are convenient because they save you from having to think about these things. Over time I’ve found that more often than not, using strictly unique_ptr for ownership, raw pointers and references to avoid copies, and plain value copies where appropriate and not performance critical, leads to code that is much easier to reason about, has less bugs, etc.

Indeed. Practical example from another language. I was using a thread pool written by someone else to execute some tasks. For reasons, the tasks were essentially represented as shared_ptr's to an object instance.

The pool dequeued a task, issued it to a thread, the thread then called a method on the object and asked for more work.

I stumbled over a case where the destructor of the task was not always called, leaving all sorts of handles hanging.

Turned out the code that dequeued a task did so into a local variable, and didn't clear that variable after it had passed it on to the thread. So when the pool ran out of work, a reference to the last task was being held by the local variable in the pool code, and that reference was only released when new work arrived or the pool was destroyed.

This is the only good use of a const& shared_ptr which is when you 1) only _might_ want to claim shared ownership of the thing, and 2) the perf of those unnecessary refcounts matter.

However, you missed a _super_ important caveat of this which is very subtle and very awful. And that is all of the code being executing here between the start of the function and the copy of the shared_ptr is doing it without the protection of the reference count on the shared pointer. It's entirely conceivable that, for example, the `has_widget` function, while executing, invalidates the original shared_ptr passed in and thus makes your eventual use of that shared_ptr invalid.

How would it invalidate it?

You would need to pass it across threads. At least that is all I can come up with.

that will not invalidate it. it might have other (negative) implications, but it does not invalidate it.

It doesn't automatically invalidate it, but it makes possible things that would invalidate it.

such as?

Other responses (different replies to grandparent) have examples.

More realisticly, if you pass a reference to a different thread and return there is no way when to know when the original goes out of scope. Don't do that.

the whole point of using shared_ptr<T> is to remove the need to "know when the original goes out of scope".

you don't need threads for what you're describing: simply passing it to a same-thread method/function which creates a heap-based copy will already complicate your understanding of the lifetime of the referenced object.

concrete example: we make wide use of the so-called "signals&slots" pattern, which is more accurately denoted as "anonymous callbacks". we have had a consistent source of serious lifetime mgmt problems caused by (boost|sigc|std)::bind()-ing a shared_ptr<T> into the argument list for the callback - the referenced object now lives for as long as the callback itself lives. This isn't explicit in the code, and even today, probably 12-15 years after we first realized the anti-pattern, we still do it from time to time.

Simple example:

    shared_ptr<int> global;
    void func(const shared_ptr<int>& v) {
        global.reset();
        *v += 1; // ohno
    }
    func(global);

fair enough, but that's fairly stupid programming :)

/* XXX the argument might be the same as the global value we're about to modify. API designers, eh? */

Doesn't have to be global - could be a member that gets modified in a callback, or that goes out of scope because something else gets deconstructed.

I'd also like to inquire as to what nontrivial codebase you've seen that contains no fairly stupid programming. I keep hearing about them, and trying to prove their existence, but so far I've collected more concrete evidence for the existence of bigfoot than I have for the existence of such codebases.

I would assume the caller itself is holding on to the shared pointer.

True but that's bug-for-bug with passing a const&

Thank you for writing that. I was about to write ... exactly the same. There is a very legitimate use case for passing the const reference to the shared pointer that the article doesn't consider at all, which you've explained very well.

I want a new term - "author smell" or "article smell".

I loathe when someone or some article rants on about their silver bullet solution, or alternatively, that doing some thing is always bad.

I learned a test taking strategy early in my life - when faced with a multiple choice question, if you see "always" or "never", that answer is usually incorrect.

To come back around to this article and similar software development articles - we have many tools in our toolbox, the trick is knowing when to use (or not use) a particular tool. Never throw your tools away.

There’s a few exceptions to your rule, too ;) Never use std::auto_ptr, for example.

I think another way to put it is that when I am calling a non-async function, for instance, I am not sharing ownership of the shared object. So calling a normal function that blocks probably would be best as the Widget ref/Widget * with/without const depending on the type of operation. But the copy of a shared_ptr<T> is saying that we are sharing ownership and the cases where that applies. An async operation, like the argument to a thread or a task, probably would require sharing ownership/lifetime of the object. Or it may be a transfer of ownership via std::move.

Reply on news.ycombinator.com