search

MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
291 stars 179 forks source link

Don't use std::move for return values #2879

Closed daljit46 closed 5 months ago

daljit46 commented 5 months ago

It prevents NRVO (Named Return Value Optimisation) and in fact can be a pessimisation. See here.

github-actions[bot] commented 5 months ago

clang-tidy review says "All clean, LGTM! :+1:"

Lestropie commented 5 months ago

Is this just a set of instances that you yourself noticed, or was it a clang tidy modify across the repo? (Not asking because of the prospect of re-authoring to MRtrixBot---that's only for instances where the changeset is disproportionate to the effort invested---asking because if it's not yet a repo-wide check, maybe we should do that)

daljit46 commented 5 months ago

I grepped for "return std::move" and these are the only instances in the codebase I could find. There are two more in core/file/json/json.h but since that's a third party header file, it wouldn't be wise to change it. Clang-tidy didn't produce any warning on my local system (although I did expect it to, but there are may be very rare cases where the use of std::move is appropriate).