[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 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.




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 implemented as a conversion operator instead of as a converting constructor. I have verified that both libstdc++ trunk and libc++ trunk implement it "correctly" as a converting constructor, and I have verified on Wandbox/Godbolt that no warning is emitted on your sample code when using either of those standard libraries.

Is it possible that you are using a standard library that is neither libc++ or libstdc++?
Is it possible that that standard library implements the iterator-to-const_iterator conversion as a conversion operator instead of a converting constructor?

> And then someone else pointed out that http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579 might mean that the code-as is should be fine too in C++14 – I don't know if that's true, but maybe you could comment on that too :-)

CWG1579 is the defect report against ISO C++11 which is mentioned in the wording of the off-by-default -Wreturn-std-move-in-c++11. :)  Anything that -Wreturn-std-move itself complains about, is stuff that was NOT fixed by CWG1579.
Alternative hypothesis: Is it possible that you have manually turned on -Wreturn-std-move-in-c++11 (which is deliberately omitted from -Wmove/-Wall)? In that case, the warning is absolutely correct, but you are probably wrong to care about fixing it in this case. :)

> Maybe the warning could suggest something like "cv qualifiers don't match, make them match" on a note in addition or instead of std::move() for this case?

I would love to see this, but I'm not comfortable trying to add it myself. Remember, the problem here isn't core-language cv-qualifiers ("iterator" vs. "const iterator"); the problem is library-level type mismatch ("iterator" vs. "const_iterator"). You'd have to somehow look at the human-readable name of the typedef and determine that it was a match for the regex /const_(.*)/ and the target type was a match for /\1/, and then you'd have to generate a meaningful note.


Repository:
  rC Clang

https://reviews.llvm.org/D43322