Task #1912
closedStop 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_ptr
by 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 over 9 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 over 9 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 over 9 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 over 9 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 over 9 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 over 9 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_ptr
parameter.- 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 about 9 years ago
Our code doesn't conform to Sutter's guidelines though. Why did you close this?
Updated by Junxiao Shi about 9 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 about 9 years ago
- Related to Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channels added
Updated by Junxiao Shi about 9 years ago
- Related to deleted (Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channels)