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

Quuxplusone added a comment. > Can you post the diagnostic exactly as it appears in the compiler output? I > am surprised that it would appear here. It should appear here only if the > standard library's implicit conversion from std::map<...>::iterator to > std::map<...>::const_iterator were

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

Quuxplusone added a comment. In https://reviews.llvm.org/D43322#1083075, @arthur.j.odwyer wrote: > Sorry, I responded via email but I guess Phabricator didn't pick it up for > some reason. > See below. And then Phab *still* didn't pick up the important part... Okay, I'll try pasting it

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

arthur.j.odwyer added a comment. Sorry, I responded via email but I guess Phabricator didn't pick it up for some reason. See below. Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

thakis added a comment. This is a cool warning, thanks for adding it. We ran into one thing while enabling this in Chromium that I'd like to mention here. We have code that basically does: struct Foo { using passwords_iterator = std::map

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC329914: Diagnose cases of return x that should be return std::move(x) for efficiency (authored by malcolm.parsons, committed by ). Repository: rC Clang https://reviews.llvm.org/D43322 Files:

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

Quuxplusone added a comment. Tuesday ping! I just need someone to commit this for me. Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Quuxplusone updated this revision to Diff 141174. Quuxplusone added a comment. Finally learned how to "make check-clang" and actually run the test in the correct environment. Had to add `-fcxx-exceptions` to the command lines at the top of that file, because the code uses `throw`. @rsmith

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

Quuxplusone added a comment. @rsmith ping? Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

Quuxplusone updated this revision to Diff 140103. Quuxplusone added a comment. Addressed review comments from @rsmith and rebased on master. I still need someone else to land this for me, though. Repository: rC Clang https://reviews.llvm.org/D43322 Files:

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

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/Sema/Sema.h:3827 bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD, - bool

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

Quuxplusone updated this revision to Diff 139961. Quuxplusone edited the summary of this revision. Quuxplusone added a comment. Rebased on master. AFAIK this is ready to go and I just need someone to land it for me... @rtrieu ping? I believe the PR summary would be suitable as a commit

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

Quuxplusone added a comment. @rtrieu gentle ping! Action items I need help with, cut-and-pasted from above: - Ideally, test compiling a bunch of (e.g. Google) code with https://reviews.llvm.org/D43322, see if there are any rough edges - Decide if `-Wmove` should imply `-Wreturn-std-move` (I

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

Quuxplusone added a comment. @rtrieu ping! I've rebased this on the pure refactoring you committed for me last week. Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Quuxplusone updated this revision to Diff 138594. Quuxplusone added a comment. Rebase on master, now that the refactoring https://reviews.llvm.org/D43898 has been pushed (thanks @rtrieu!) Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td

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

Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:388 def PessimizingMove : DiagGroup<"pessimizing-move">; +def ReturnStdMoveInCXX11 : DiagGroup<"return-std-move-in-c++11">; +def ReturnStdMove : DiagGroup<"return-std-move">;

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

lebedev.ri added a comment. In https://reviews.llvm.org/D43322#1037889, @Quuxplusone wrote: > The backward-compatibility-concerned diagnostic, > `-Wreturn-std-move-in-c++11`, is *not* appropriate for `-Wmove`; Have you evaluated possibility of adding `-Wreturn-std-move-in-c++11` to one of

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

Quuxplusone updated this revision to Diff 138457. Quuxplusone added a comment. Boldly add `-Wreturn-std-move` to `-Wmove` (and thus also to `-Wall`). The backward-compatibility-concerned diagnostic, `-Wreturn-std-move-in-c++11`, is *not* appropriate for `-Wmove`; but I believe this main

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

Quuxplusone updated this revision to Diff 137921. Quuxplusone added a comment. Rebased on https://reviews.llvm.org/D43898 after addressing @rtrieu's latest comments over there. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td

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

Quuxplusone added a comment. Btw, I'm going to be talking about this patch tonight at 7 in Palo Alto. :) https://www.meetup.com/ACCU-Bay-Area/events/248040207/ https://docs.google.com/presentation/d/18ZRnedocXSQKn9Eh67gGv-ignReHfRD7vj_dxrra1kc/ Repository: rC Clang

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

Quuxplusone added a comment. @rtrieu (and perhaps @rsmith) ping? The action items I need help with are: - Review and land the refactoring patch https://reviews.llvm.org/D43898 (I don't have commit privs) - Ideally, test compiling a bunch of (e.g. Google) code with

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

Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5639 + "local variable %0 will be copied despite being %select{returned|thrown}1 by name">, + InGroup, DefaultIgnore; +def note_add_std_move : Note< I would like some

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

Quuxplusone updated this revision to Diff 136383. Quuxplusone added a comment. Rename some functions and parameters. Rebase onto https://reviews.llvm.org/D43898. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td

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

Quuxplusone added inline comments. Comment at: lib/Sema/SemaStmt.cpp:2937 +static void AttemptMoveInitialization(Sema& S, + const InitializedEntity , rtrieu wrote: > I have a few concerns about this function. The code

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

rtrieu added a comment. > I have one very minor nit that I don't know how to fix: > > warn-return-std-move.cpp:220:12: warning: local variable 'd' will be copied > despite being returned by name [-Wreturn-std-move] > return (d); // e17 > ^ >

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

Quuxplusone added a comment. @rtrieu please take a look? Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

Quuxplusone added a comment. In https://reviews.llvm.org/D43322#1017190, @lebedev.ri wrote: > Somewhat related thing i have noticed: https://godbolt.org/g/ow58JV IIUC, you're asking whether it would be possible to detect instances of return take(mysharedptr); and rewrite them into

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

lebedev.ri added a comment. Somewhat related thing i have noticed: https://godbolt.org/g/ow58JV I wonder whether that also should be diagnosed? Where though, as clang-tidy check? Repository: rC Clang https://reviews.llvm.org/D43322 ___

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

Quuxplusone updated this revision to Diff 135484. Quuxplusone added a reviewer: rsmith. Quuxplusone added a subscriber: Rakete. Quuxplusone added a comment. Eliminate a couple of `auto` per comment by @Rakete. Repository: rC Clang https://reviews.llvm.org/D43322 Files:

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

Quuxplusone updated this revision to Diff 135005. Quuxplusone added a comment. Removed a redundant check for LValueReferenceType in the CWG1579 codepath. (In that branch, we know that standard C++ *did* perform the copy-to-move transformation, so obviously we can't have had an lvalue reference

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

Quuxplusone added a comment. @rsmith and/or @rtrieu, please take another look? All my TODOs are done now: there are fixits, and the wording of the diagnostic changes if it's a "throw" instead of a "return", and the wording has been updated per Richard Smith's suggestions. I have one very

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

Quuxplusone updated this revision to Diff 134999. Quuxplusone edited the summary of this revision. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

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

Quuxplusone updated this revision to Diff 134779. Quuxplusone added a comment. Add fixits. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

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

Quuxplusone updated this revision to Diff 134514. Quuxplusone added a comment. Reword the diagnostic per rsmith's suggestions. Still TODO: - figure out how to detect whether this is a return-stmt or throw-expression - figure out how to generate the appropriate fixit Repository: rC Clang

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

Quuxplusone marked an inline comment as done. Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:23 +def warn_return_std_move : Warning< + "adding 'std::move' here would select a better conversion sequence">, + InGroup, DefaultIgnore;

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

rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:23 +def warn_return_std_move : Warning< + "adding 'std::move' here would select a better conversion sequence">, + InGroup, DefaultIgnore; Can we say something like "local

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

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,