In D137205#4047828, @Skylion007 wrote:

I am trying this in the wild and getting some false positives where it tries to call std::move inside loop conditions and in the boolean condition for an if statement. Stuff like:

if (SmartPtr new_ptr = steal_ptr(std::move(old_ptr))) {
  ...
}
else {
  return old_ptr; // Now it's moved from and invalid
}

Also this can be happen for similar boolean conditionals in while, for, do while loops.

Interestingly, the logic in bugprone-use-after-move flags this error so maybe we can reuse some of that logic to detect bad std::move.

In D141058#4028768, @v1nh1shungry wrote:

Sorry, I don't know how to add tests for such fixes. Could anyone please give me some hints? Thanks!

In D137205#4018548, @ccotter wrote:

@Febbe - Really glad to see this phab up! Not having realized this phab was in progress, I implemented this a few months back (though, originally not as a clang-tidy tool and never published) and thought it'd be worth sharing notes. I recently turned my non-clang-tidy implementation into a clang-tidy one here. A couple suggestions based on my experience writing a similar tool,

It is also completely irrelevant, because a new programmer will not understand that const T const * is actually T const* and not T const * const. An experienced programmer can understand it well either way.

not sure I follow the argument here, but surely I also agree that east consts are more readable and fool-proof. it's unfortunate that most of the C++ world is stuck with the west consts.

Applied clang-format QualifierAlignment: Left

Fixed typo

In D137205#3950722, @kadircet wrote:

another thing that i noticed is usage of east consts in the implementation files. no one seem to have brought it up so far (at least none that i can see).
in theory there are no explicit guidelines about it in LLVM coding style, but rest of the codebase uses west const convention. so i am not sure if straying away from it here will make much sense.

Replaces match clauses with RecursiveASTVisistors

• doubled performance
• fixed also a bug in template spezialisations

Improved fixit suggestion.

• No duplicated replacements.

Removed replacements for macros, since they could be wrong somehow.
Made some helperfunction non-trailing

In D137205#3923643, @Skylion007 wrote:

The main false positive I also keep seeing is in pybind11 where it suggests call an std::move() for an implicit conversion, but the target of the implicit conversion does not have an rvalue. (In this case, we have a py::object which inherits from py::handle, and is just py::handle with ownership semantics. This allows implicit conversion to a reference at anytime, and therefore std::move has no effect here except to complicate the code a bit.

In D137205#3936944, @firewave wrote:

Here's another false positive:

#include <string>

void f(const std::string&);

int main() {
    std::string s;
    f(s.empty() ? "<>" : s);
}

input.cpp:7:26: warning: Parameter 's' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
    f(s.empty() ? "<>" : s);
                         ^
                         std::move( )

In D137205#3936944, @firewave wrote:

The crash is gone.

The false positive with the [m] capture is still present with -std=c++11.

fixed lamda detection

I have a problem with lambdas capturing by copy implicitly ([=]()...). It seems like, that child nodes are not reachable via a MatchFinder unless they are spelled in source. I actually don't know how to prevent a fix elegantly: My proposed idea is, to check the source location of the capture list for each lambda and check, if the declRefExpr is part of it... . This is bug prone, and possibly slow.

Fixed some false positives:

• no move for no automatic storage duration
• no move for lambda captures

In D137205#3920016, @firewave wrote:

Getting this false positive:

#include <string>

extern std::string str()
{
	std::string ret;
	return ret.empty() ? ret : ret.substr(1);
}

input.cpp:6:23: warning: Parameter 'ret' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
        return ret.empty() ? ret : ret.substr(1);
                             ^
                             std::move( )

Appears to be caused by the ternary since I also have it inline with a function parameter in another case.

Fixed segfaults due to asserts which were wrongly assumed to be always true

In D137205#3915526, @Skylion007 wrote:

Okay, now I am getting what I believe to be segfaults:

#0 0x0000564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
#1 0x0000564383480464 SignalHandler(int) Signals.cpp:0:0
#2 0x00007f7c275c9420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
#3 0x00005643804b0ea5 clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) (.cold) UnnecessaryCopyOnLastUseCheck.cpp:0:0
#4 0x000056438262bba1 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) ASTMatchFinder.cpp:0:0
#5 0x00005643826818df clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) (/home/aaron/git/llvm-project/build/bin/clang-tidy+0x2d258df)

It worked before this PR, now it just crashes on a lot of real world codebases including just trying to run it for a few files on LLVM's own codebase.

Added some tests. Code cleanup.

Fixed lValueReference detection in matcher

In D137205#3912243, @Skylion007 wrote:

One other bug I found with this diff, is it seems to suggest calling std::move() on function args that are references, despite the fact that invalidating the reference to the input arg could be undesirable. For instance take a function that takes in a reference to a String, assigns a new value to the arg reference, and then returns the value of the reference. It suggests calling std::move() on the arg ref when it is returned, which invalidate the reference to the argument.

So I finally had time to apply your feedback.

Applied feedback

• replaced some auto types with the actual type
• added IncludeStyle to the options list in the documentation
• Added "Limitations" paragraph, describing known limitations

I applied the rest of your feedback.
There are other usages of auto like auto FoundUsage which is a Usage for example. ~Shall I also replace those obvious cases?~

Applied feedback.

Applied feedback,
Also added the documentation for the second option `BlockedFunctions`

Added documentation

I also have to add the http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-copy-on-last-use.html page, but I don't have any clue how, and where to start.

@salman-javed-nz you're welcome, I only fixed it because I saw the bug in the trace directly. Normally, I would only fix C/C++ stuff :D.

@JonasToth yes, it would be nice, to test this and then push it for me. Also a backport to 14.0 would be good :).

Applied the feedback

I don't think I have commit rights, so someone of you also have to commit this.

Currently, only tested on Windows.
This should also increase performance, since the path is canonicalized only once.

Thank you for the review. I am done and you can commit the patch :) . I don't have the rights to commit.

Last batch of suggested changes

Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)?
Also, is the commit added automatically to the repo, or do I / another one have to rebase it.

Applied feedback

Push, would be nice to see this in the llvm-14 release. I can also do a review for someone else as a favor.

Updated in https://reviews.llvm.org/D118847