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

[Richard Trieu via Phabricator via cfe-commits]

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.cpp:220:12: note: call 'std::move' explicitly to avoid copying
>       return (d);  // e17
>              ^~~
>              std::move(d)
> 
> 
> The warning places a caret directly under the `(`, when I wish it would place `^~~` under the entire expression, the way the fixit does.

You can add extra tildes to any diagnostic by passing a SourceRange to Diag() calls the same way you pass FixitHints.



================
Comment at: lib/Sema/SemaStmt.cpp:2937
 
+static void AttemptMoveInitialization(Sema& S,
+                                      const InitializedEntity &Entity,
----------------
I have a few concerns about this function.  The code isn't a straight move from the old location to this one, so it is hard to follow along the changes, especially the change to the complex if conditional.  The function should be commented, especially to behavior difference for setting IsFake.

It looks like when IsFake is set, then the result is discarded and not used, but it is still possible on success for AsRvalue to be copied to the heap.  This is wasteful when it is never used again.

Another issue is the Value in the original code is used again towards the end of the function on line #3013.  In the old code, Value can be updated while in the new code, it does.

It may be better to split this change in two, the first adding this function and the CopyElisionSemanticsKind enum and the second adding the diagnostic itself.


Repository:
  rC Clang

https://reviews.llvm.org/D43322