Project

General

Profile

Actions

Task #1912

closed

Stop using const shared_ptr<T>& parameter type

Added by Junxiao Shi over 9 years ago. Updated about 9 years ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Category:
Utils
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

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

sharedptr-bench.cpp (779 Bytes) sharedptr-bench.cpp Junxiao Shi, 08/22/2014 09:38 PM
Actions #1

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.

Actions #2

Updated by Junxiao Shi over 9 years ago

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.

Actions #3

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.

Actions #4

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.

Actions #5

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.

Actions #6

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 use T* instead (or if not nullable, a T&).

The whole article is worth a read.

Actions #7

Updated by Junxiao Shi over 9 years ago

I agree with the idea in note-6.

Actions #8

Updated by Junxiao Shi about 9 years ago

  • Status changed from New to Rejected

See note-6.

Actions #9

Updated by Davide Pesavento about 9 years ago

Our code doesn't conform to Sutter's guidelines though. Why did you close this?

Actions #10

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.

Actions #11

Updated by Davide Pesavento about 9 years ago

Ok, fine by me.

Actions #12

Updated by Davide Pesavento about 9 years ago

  • Related to Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channels added
Actions #13

Updated by Junxiao Shi about 9 years ago

  • Related to deleted (Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channels)
Actions

Also available in: Atom PDF