Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise arguments which were being passed by value instead of as a reference #2668

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

SunBlack
Copy link
Contributor

Fixed all passedByValue hints by CppCheck 1.85

@jasjuang
Copy link
Contributor

jasjuang commented Dec 1, 2018

I believe pass vectors by reference makes sense but strings should be passed by value because of SSO.

Please see https://stackoverflow.com/a/10232761/3667089

@SergioRAgostinho
Copy link
Member

@jasjuang What the article you pointed is saying is that if you need to create a copy of the string, inside the function, for any given reason, then you should definitely pass by value.

simulation/tools/sim_viewer.cpp Outdated Show resolved Hide resolved
gpu/kinfu/tools/camera_pose.h Show resolved Hide resolved
@SergioRAgostinho
Copy link
Member

Nb: I'm not marking this as an ABI change because it is just targeted at apps and tools. Nothing on the main libs public interface.

@SergioRAgostinho SergioRAgostinho merged commit c6a10da into PointCloudLibrary:master Dec 3, 2018
@SergioRAgostinho SergioRAgostinho changed the title Pass values by reference Revise arguments which were being passed by value instead of as a reference. Dec 3, 2018
@jasjuang
Copy link
Contributor

jasjuang commented Dec 3, 2018

@SergioRAgostinho Yes, but that's an extra precaution that needs to be taken into account. Passing in small strings as value is not more expensive because of SSO. My point is there is one more boundary condition for const & to check for verses pass by value yet there isn't any gain over it, so why bother to make the change?

@SergioRAgostinho
Copy link
Member

If a string meets the SSO length criteria, then it is allocated on the stack. Great for constructors and move mechanics. It's good that it exists when we qualify for it, but I can only rely on the assumptions that my string is going to be small in a couple of controlled cases. This lenght criteria is implementation defined but for libc++ appears to be 3 words, and according to the answer yields 10 chars on a 32 bit machine, 22 chars on a 64bit. So you're copying 24bytes vs a pointer (8 bytes) in the best case.

The claimed benefit is that one doesn't need to care about it anymore because in most cases SSO will kick in and it's a minor stack copy, if the string is short. Also you don't have to write the extra const and &.

In the opposite case, you obviously write const & but what exactly is the downside of this approach?

@jasjuang
Copy link
Contributor

jasjuang commented Dec 3, 2018

If we can't assume the string to be small I agree const & is a better idea. Thanks for showing me the link. That was an interesting read!

@SunBlack SunBlack deleted the passedByValue branch December 5, 2018 12:33
@taketwo taketwo changed the title Revise arguments which were being passed by value instead of as a reference. Revise arguments which were being passed by value instead of as a reference Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants