Task #1912
closed
Stop using const shared_ptr<T>& parameter type
0%
Description
Some code is declaring const shared_ptr<T>& as parameter type.
This is potentially bad, because:
- It's risky to pass
shared_ptrby const reference, because there is no guarantee that the pointer would remain valid throughout the function call: the function may have a side effect of destroying the pointer. ref - Passing by const reference instead of passing by value is a performance optimization, but this optimization is premature because it's not justified by benchmark.
We should stop adding new functions that declares const shared_ptr<T>& parameter type, and gradually change existing functions to pass shared_ptr by value.
Files
Updated by Alex Afanasyev about 10 years ago
I disagree. I have checked on the issue before. The only "danger" is when you do circle assignments and this is not normal case in the first place.
We should use const& everywhere we are using right now and may be convert places where we don't.
Updated by Junxiao Shi about 10 years ago
- File sharedptr-bench.cpp sharedptr-bench.cpp added
Benchmark results on Ubuntu 12.04:
- passing by const reference: 7422122ns per million calls
- passing by value: 46806502ns per million calls
There is a performance benefit of 39ns per call.
The question is: is this small performance benefit worth sacrificing the long-term maintainability?
Code should be defensive when possible.
Updated by Alex Afanasyev about 10 years ago
Yes, there is benefit and it is small only on surface. Also, there is no real harm to maintainability.
Updated by Junxiao Shi about 10 years ago
See this answer on why code using const shared_ptr<T>& would have maintainability issues: it relies on the calle not to decrement the reference count of the parameter, which cannot be guaranteed or proved in general case.
Updated by Anonymous about 10 years ago
One of the alternate answers to that stack overflow question strongly refutes the one you linked and cites Herb Sutter and Scott Meyer:
http://stackoverflow.com/a/8844924
direct link to Sutter and Meyer discussing shared_ptr: http://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2011-Scott-Andrei-and-Herb-Ask-Us-Anything#time=04m34s
This project is my first time using shared_ptrs, but I think the take away is that we shouldn't worry about the general case for every usage. There will be cases where we need to worry about the inc/dec, but those should be handled on a case by case basis.
Updated by Davide Pesavento about 10 years ago
Sutter recommends a different approach, as explained here http://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/. The idea is basically:
- Don’t pass a smart pointer as a function parameter unless you want to use or manipulate the smart pointer itself, such as to share or transfer ownership.
- Prefer passing objects by value,
*, or&, not by smart pointer.- Express that a function will store and share ownership of a heap object using a by-value
shared_ptrparameter.- Use a
const shared_ptr<T>&as a parameter only if you’re not sure whether or not you’ll take a copy and share ownership; otherwise useT*instead (or if not nullable, aT&).
The whole article is worth a read.
Updated by Davide Pesavento almost 10 years ago
Our code doesn't conform to Sutter's guidelines though. Why did you close this?
Updated by Junxiao Shi almost 10 years ago
Our code doesn't conform to Sutter's guidelines though. Why did you close this?
This issue does not have a measurable goal, and thus is not constructive.
Future code review shall enforce the guidelines cited in note-6.
Updated by Davide Pesavento over 9 years ago
- Related to Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channels added
Updated by Junxiao Shi over 9 years ago
- Related to deleted (Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channels)