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 im
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 here.
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
http:
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,
ReverseStr
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:
in
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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe
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 PTAL?
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
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:
include/clang/Basic/DiagnosticGrou
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 AllowParamOrMoveCons
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 message.
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 hav
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
http://lists.llvm.o
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
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">;
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
`CX
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 diagnosti
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
include/cl
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
https://reviews.llvm.org/D4
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
https://reviews.llvm.org/D433
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 gui
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
include/clang/Basic/Diagn
Quuxplusone added inline comments.
Comment at: lib/Sema/SemaStmt.cpp:2937
+static void AttemptMoveInitialization(Sema& S,
+ const InitializedEntity &Entity,
rtrieu wrote:
> I have a few concerns about this function. The cod
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
> ^
> warn-return-std-move.
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
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
return
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
___
cfe-commi
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:
include/clan
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 t
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 mino
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
lib/Sema/SemaExprC
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
lib/Sema/SemaExprCXX.cpp
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
http
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;
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 varia
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,
36 matches
Mail list logo