HomePhabricator

Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Description

Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

-Wreturn-std-move

This diagnostic is enabled by -Wreturn-std-move, -Wmove, or -Wall.
Diagnose cases of return x or throw x, where x is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is stdext::inplace_function<Sig, N> which implements conversion via a conversion operator rather than a converting constructor; see https://github.com/WG21-SG14/SG14/issues/125#issue-297201412
Another place where this has come up in the wild, but where the fix ended up being different, was

try { ... } catch (ExceptionType ex) {
    throw ex;
}

where the appropriate fix in that case was to replace throw ex; with throw;, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

-Wreturn-std-move-in-c++11

This diagnostic is enabled only by the exact spelling -Wreturn-std-move-in-c++11.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

Discussion

Notice that this patch is kind of a negative-space version of Richard Trieu's -Wpessimizing-move. That diagnostic warns about cases of return std::move(x) that should be return x for speed. These diagnostics warn about cases of return x that should be return std::move(x) for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a return statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user would see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit std::move only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D43322

Patch by Arthur O'Dwyer.

Details

Committed
malcolm.parsonsApr 12 2018, 7:48 AM
Reviewer
rsmith
Differential Revision
D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Branches
Unknown
Tags
Unknown