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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone created this revision.
Quuxplusone added a project: clang.
Herald added a subscriber: cfe-commits.

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

**-Wreturn-std-move**

Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy is made.
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.)

**-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. So I'm using the CWG issue number in the diagnostic but "C++11" in the name of the option. 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."

**Discussion**

I fully expect there to be lots of discussion on this patch, and for my first draft to be missing lots of pieces. I do hope that we can hammer it into a shape suitable for upstreaming, but milestone number 1 is just to get it out there for people to use on their own codebases.

Notice that this patch is kind of a belated 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.)


Repository:
  rC Clang

https://reviews.llvm.org/D43322

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaStmt.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/SemaCXX/warn-return-std-move.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43322.134327.patch
Type: text/x-patch
Size: 26576 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180214/2fd30dcd/attachment-0001.bin>