[clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Febbe]

Summary

• finds occurences of last uses, which are copied.
• suggests adding a std::move.
• detects assignments as last use.
• Added BlockLists for false positive types and functions

[Febbe]

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.

[Febbe]

Added documentation

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

2

Please make single line.

229

Please don't use auto unless type is explicitly spelled in same statement or iterator.

230

Ditto.

258

Ditto.

274

Ditto.

275

Ditto.

358

Please fix.

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

7

Please synchronize with Release Notes, follow 80 characters limit and use double back-ticks for language constructs.

10

Double back-ticks.

13

Excessive newline.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

5

Please make same length as title.

[Febbe]

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

7

Do you mean, to add this short description to the release notes?
Is the 80 characters limit for the line, or the whole description?

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

7

I meant that first sentence in documentation should be same as Release Notes record.

80-characters limit should be followed in code and documentation.

[Febbe]

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

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

2

Please put space after clang-tidy.

clang-tools-extra/docs/ReleaseNotes.rst

124

One sentence is enough.

[Febbe]

Applied feedback.

[Febbe]

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?~

• I just checked the guidelines. Obvious cases can be auto.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h

2

Sorry, overlooked last time. Please add space after `clang-tidy'.

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

33

Please use single back-ticks for option values. Same below.

[njames93]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

8

Author ditto.

48

These should be static

53

This doesn't belong in the final version of this patch.

60

Anonymous namespaces should be kept as small as possible and only used for decls that don't have another way to specify linkage. Therefore these functions should be moved out of the namespace and marked as static.

112

Is this an expected issue to run into, if not this should be seen assert. Same goes for below.
Also given no user would ever see the error message, it's probably unnecessary to create them. If the errors are situations that are expected to happen during runtime, make this function return an optional, if not just return a Usage directly.

142

replace auto with type here.

144

Ditto

215

AsIs traversal is the default kind, so there's no need to specify this.

237

Isn't blocked types meant to be a regular expression?

273

This code can be moved inside the template check then branch as it's not used in the else or after the if.

295

What's this comment for?

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h

8

We don't include author information in files, that is kept in the got history instead.

11

Remove this, the include guard is enough.

22

I have no preference either way, but can these namespaces all be nested or none nested.

28

34

You need at least c++11 as that's when move was introduced.

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

4

We avoid using the standard library for tests, if you need to use anything from it you have to reimplement the parts you need yourself.

[Skylion007]

Use llvm::find_if instead of std::find_if

[Skylion007]

I noticed one other bug from testing this out on some C++ codebases.

Specifically in pybind11 we have an object class which is a child of the handle object. The object class is non-trivially copyable, while the handle object is not. Therefore, when we do not want to increase / decrease the reference count of object, we often rely on object's implicit casting to a handle. While I suppose you could std::move() to cast an object for a handle, there isn't a good reason to do so. You get all the drawbacks of using std::move (ie. use after free bugs potentially) with none of the benefits.

Finally, this is a smaller issue but there seems to be a bug where the fix it can apply std::move(std::move(obj));. That is relatively low pri and not blocking by any means though.

[Febbe]

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

[Febbe]

So I finally had time to apply your feedback.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

142

Unused, because inlined next line.

237

It is also handled twice, therefor I just removed it.

295

Removed, old Todo I have overseen.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h

11

Yes, I thought, #pragma once is less bug prone and might be faster.

22

Had a forward declaration there, and added it again to reduce include dependencies.

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

4

Kept the static_assert as comment and removed the header.

[Skylion007]

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.

[Febbe]

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.

Oh, that's not good. Actually, it should not run on references, only declRefExpr(hasType(qualType(unless(anyOf(isConstQualified(), referenceType(), pointerType() ,..) are allowed to be parsed.

Did you run this check alone, without other checks?

Regarding the appearance of std::move(std::move( var );, was there a move already? Does it occur in a header file? In case of running clang-tidy with fixups in parallel over multiple sources, it can happen, that a header file is parsed twice at the same time, resulting in duplicate insertions.

And last but not least, can you provide a source snipped or AST for both appearances?

[Skylion007]

In D137205#3913192, @Febbe wrote:

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.

Oh, that's not good. Actually, it should not run on references, only declRefExpr(hasType(qualType(unless(anyOf(isConstQualified(), referenceType(), pointerType() ,..) are allowed to be parsed.

Did you run this check alone, without other checks?

Regarding the appearance of std::move(std::move( var );, was there a move already? Does it occur in a header file? In case of running clang-tidy with fixups in parallel over multiple sources, it can happen, that a header file is parsed twice at the same time, resulting in duplicate insertions.

And last but not least, can you provide a source snipped or AST for both appearances?

The problematic code snippit can be found here: https://github.com/facebookresearch/habitat-sim/blob/c88851802f1905ab483e380367753dc6a12c6d21/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h#L323

And results in this diff:

diff --git a/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h b/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
index d5564c8f..37a38b00 100644
--- a/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
+++ b/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
@@ -10,6 +10,8 @@
  * esp::metadata::managers::AbstractObjectAttributesManager
  */

+#include <utility>
+
 #include "esp/metadata/attributes/ObjectAttributes.h"

 #include "esp/metadata/MetadataUtils.h"
@@ -312,7 +314,7 @@ AbstractObjectAttributesManager<T, Access>::setJSONAssetHandleAndType(
     const std::function<void(int)>& meshTypeSetter) {
   std::string propertiesFileDirectory = attributes->getFileDirectory();
   // save current file name
-  std::string oldFName(assetName);
+  std::string oldFName(std::move(assetName));
   // clear var to get new value - if returns true use this as new value
   assetName = "";
   // Map a json string value to its corresponding AssetType if found and cast to
@@ -362,7 +364,7 @@ AbstractObjectAttributesManager<T, Access>::setJSONAssetHandleAndType(
                                            meshTypeSetter);
       }
     }  // value is valid prim and exists, else value is valid file and exists
-    return assetName;
+    return std::move(assetName);
   }
   // handle value is not present in JSON or is specified but does not change
   return oldFName;

[Febbe]

Fixed lValueReference detection in matcher

[Febbe]

Added some tests. Code cleanup.

[Skylion007]

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.

[Febbe]

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.

I thought I had fixed it with my newest diff.
The reason for the segfaults are, that I removed the llvm::Expected and replaced it with asserts. But there are cases, where the asserts can be false with regular (currently unhandled code).

I also found issues:

• a fixup is proposed even when the copy-parameter is not mentioned in the code. This is the case for capturing lambdas ... = [=]{...}; -> ... = [std::move(=)]{...};
• a fixup is proposed for trivial types like std::pair<unsigned, unsigned>

[Febbe]

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

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

41

Please separate with newline.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

51

Could such option be shared between different checks? Or even rely on .clang-format?

72

Please unindent. It's not a code.

78

Please add.

[Skylion007]

Currently, this check also tries to move static values which is very undesirable and unlikely to be correct.

   static auto value = std::string("CONSTANT");
-  return value;
+  return std::move(value);

[Skylion007]

Hmm, I still had a crash on this latest code (although it is crashing much more rarely now.

It crashes on scanning this file https://github.com/facebookresearch/habitat-sim/blob/5fb04078b0b8432dc4a88ec186a2b7af74163be1/src/esp/core/managedContainers/ManagedContainerBase.cpp

Found compiler error(s).
/home/aaron/git/llvm-project/build/bin/clang-tidy -checks=-*,*perf*last* -export-fixes /tmp/tmp38bqy_w9/tmpa_p4xe5f.yaml -p=build /home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/aaron/git/llvm-project/build/bin/clang-tidy -checks=-*,*perf*last* -export-fixes /tmp/tmp38bqy_w9/tmpa_p4xe5f.yaml -p=build /home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp
1.	<eof> parser at end of file
2.	ASTMatcher: Processing 'performance-unnecessary-copy-on-last-use' against:
	CXXConstructExpr : </home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp:95:56>
--- Bound Nodes Begin ---
    constructExpr - { CXXConstructExpr : </home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp:95:56> }
    param - { DeclRefExpr : </home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp:95:56> }
--- Bound Nodes End ---
 #0 0x000055a33b1c14f4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x000055a33b1bed74 SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f80af664420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
 #3 0x000055a338761176 clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) (/home/aaron/git/llvm-project/build/bin/clang-tidy+0x10c7176)

[firewave]

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.

[firewave]

Another false positive:

#include <unordered_map>

void evaluateLibraryFunction()
{
	std::unordered_map<int, const void*> m;

	auto f = [m]() {};
}

input.cpp:7:12: warning: Parameter 'm' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
        auto f = [m]() {};
                  ^
                  std::move( )

This is suggested regardless of the C++ standard defined (unfortunately I didn't get a -Wc++14-extensions warning out of clang-tidy).

Also the fix-it will result in invalid code.

[firewave]

I am also experiencing a crash:

#include <string>
#include <list>

class Token;

class Value {
		public:
		using ErrorPathItem = std::pair<const Token *, std::string>;
		using ErrorPath = std::list<ErrorPathItem>;

		explicit Value(long long val = 0)
		{}

		Value(const Token* c, long long val);

		ErrorPath errorPath; // removing this fixes the crash
};

void setTokenValue(Value value);

template<class ContainerOfValue>
static void valueFlowForwardConst(const ContainerOfValue& values)
{
	[&] {
		for (Value value : values) {
			setTokenValue(value);
		}
	}();
}

Stack dump:
0.      Program arguments: /mnt/s/GitHub/llvm-project/build/bin/clang-tidy -checks=-*,-clang-analyzer-*,performance-unnecessary-copy-on-last-use -fix /mnt/s/___temp/input.cpp
1.      <eof> parser at end of file
2.      ASTMatcher: Processing 'performance-unnecessary-copy-on-last-use' against:
        CXXConstructExpr : </mnt/s/___temp/input.cpp:26:18>
--- Bound Nodes Begin ---
    constructExpr - { CXXConstructExpr : </mnt/s/___temp/input.cpp:26:18> }
    param - { DeclRefExpr : </mnt/s/___temp/input.cpp:26:18> }
--- Bound Nodes End ---
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamEi+0x23)[0x7f2fcfefa393]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN4llvm3sys17RunSignalHandlersEv+0xee)[0x7f2fcfef82ee]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3e3582f)[0x7f2fcfefa82f]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x14420)[0x7f2fcc06a420]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang4tidy11performance29UnnecessaryCopyOnLastUseCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0xfe)[0x7f2fcd598d5e]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffab94)[0x7f2fcf0bfb94]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang12ast_matchers8internal21BoundNodesTreeBuilder12visitMatchesEPNS2_7VisitorE+0x11c)[0x7f2fcf0ecdbc]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffa5be)[0x7f2fcf0bf5be]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3016505)[0x7f2fcf0db505]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x300b8a2)[0x7f2fcf0d08a2]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3009a73)[0x7f2fcf0cea73]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3009a73)[0x7f2fcf0cea73]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x30277c7)[0x7f2fcf0ec7c7]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x3002c02)[0x7f2fcf0c7c02]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffd4f3)[0x7f2fcf0c24f3]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x30005b1)[0x7f2fcf0c55b1]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffd2cb)[0x7f2fcf0c22cb]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x300522b)[0x7f2fcf0ca22b]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x2ffd7b1)[0x7f2fcf0c27b1]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang12ast_matchers11MatchFinder8matchASTERNS_10ASTContextE+0x357)[0x7f2fcf095f77]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang17MultiplexConsumer21HandleTranslationUnitERNS_10ASTContextE+0x2c)[0x7f2fce3dde8c]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang8ParseASTERNS_4SemaEbb+0x33f)[0x7f2fce5f4f5f]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang14FrontendAction7ExecuteEv+0x59)[0x7f2fce3a2799]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang16CompilerInstance13ExecuteActionERNS_14FrontendActionE+0x316)[0x7f2fce3151a6]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling21FrontendActionFactory13runInvocationESt10shared_ptrINS_18CompilerInvocationEEPNS_11FileManagerES2_INS_22PCHContainerOperationsEEPNS_18DiagnosticConsumerE+0x1ad)[0x7f2fcddc1d9d]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(+0x1cc85e6)[0x7f2fcdd8d5e6]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling14ToolInvocation13runInvocationEPKcPNS_6driver11CompilationESt10shared_ptrINS_18CompilerInvocationEES7_INS_22PCHContainerOperationsEE+0x11a)[0x7f2fcddc1afa]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling14ToolInvocation3runEv+0x56c)[0x7f2fcddc092c]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang7tooling9ClangTool3runEPNS0_10ToolActionE+0xf9b)[0x7f2fcddc34ab]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang4tidy12runClangTidyERNS0_16ClangTidyContextERKNS_7tooling19CompilationDatabaseEN4llvm8ArrayRefINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENS7_18IntrusiveRefCntPtrINS7_3vfs17OverlayFileSystemEEEbbNS7_9StringRefE+0x3f7)[0x7f2fcdd88ba7]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_ZN5clang4tidy13clangTidyMainEiPPKc+0x2eaa)[0x7f2fcd0b9b8a]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7f2fcbaa4083]
/mnt/s/GitHub/llvm-project/build/bin/clang-tidy(_start+0x2e)[0x7f2fcd0b4b4e]

I did not build with debug symbols. Sorry about that.

[Febbe]

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.

As I know, the compilers can't copy elide always on ternary operators. Therefore, I have also a test, suggesting it on return.

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

51

I just oriented myself at other similar checks, which introduced this. It uses both there:

UnnecessaryCopyOnLastUseCheck::UnnecessaryCopyOnLastUseCheck(
    StringRef Name, ClangTidyContext *Context)
    : ClangTidyCheck(Name, Context),
      Inserter(Options.getLocalOrGlobal("IncludeStyle",
                                        utils::IncludeSorter::IS_LLVM),

Do you mean, that I can just remove the description of the option, because it is taken from global options?

[njames93]

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

51

I'm personally of the idea that it should be removed. However that's out of the scope for this patch so for now thist should stick with the current convention.

[Skylion007]

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.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

115–116

This assertion is not valid. The crash I linked above is due to "TheCFG" sometimes being null unfortunately. Making it return Usage::Error stops the crash at least. Seems to happen when it cannot find an include .

[Febbe]

Fixed some false positives:

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

[Febbe]

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.

[Febbe]

fixed lamda detection

[firewave]

The crash is gone.

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

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( )

I still have another false positive with static variables but have not gotten around reducing it yet.

I posted the false negatives in the GitHub ticket as those do not relate to getting this approved.

[Febbe]

In D137205#3936944, @firewave wrote:

The crash is gone.

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

Does only a warning appear? Or also a fix? Currently, only a warning should appear, but this is not fixable in c++11.

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( )

Thanks for the minimal examples. Need to look into the AST's.

I still have another false positive with static variables but have not gotten around reducing it yet.

I posted the false negatives in the GitHub ticket as those do not relate to getting this approved.

Also thanks, I've looked over them, seems like there is a problem with the detection in the CFG regarding self assignments.

[firewave]

In D137205#3937298, @Febbe wrote:

Does only a warning appear? Or also a fix? Currently, only a warning should appear, but this is not fixable in c++11.

There's no fix-it. I didn't realize that before.

Maybe this should also be added to the documentation as a known limitation.

[Febbe]

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( )

Actually, this is correct:

https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIAKykrgAyeAyYAHI%2BAEaYxCBmAOykAA6oCoRODB7evgGp6ZkCoeFRLLHxSbaY9o4CQgRMxAQ5Pn6BdpgOWQ1NBCWRMXEJyQqNza15HeP9YYPlw0kAlLaoXsTI7BzmAMxhyN5YANQmO25OY8SYrKfYJhoAgvcPl14OR24pJolWj0f/HxSEDG6BAIEuYWAR2YbCWRxAMMwEERcO%2BFhBYLQXgIJzOpzcJzMZjQDFe73MZlxbnx0NYmCpNIpJn8bgYTJ2VkSABFngCjgA/T4QVE/DEoNY4/GMolYMkECkMs60tiKglMllsomnTk8x68gFCz5HEljcwANmNSzR%2Br5/wICDwCgAtLdEbiucaAHRu6yEswmM0adkWE5/W3/MVYyV4pUUtApACeftVXp9MbVRKORGTUqV9sdLp22DT1NjWo1we%2BuoefJtgIgRvN5st1rD4fzztddPdRzFLFQADckchvXSrRy63zIxKUxT%2B0OFbmCSOS9L/RWtRO2yduc860xsagjqgUnEmERiKcuQ2UsaBKazBbkFbRQRQeLsbOifGk0wFBlgDZDd/QnXdtwPbMTzPC8rxvQkzWbZ80WnT8lz9ed6T/ACgNZSswKebcxQhIxlUwbU913LcCIeAdUDwdBjWIJhkE2FICDgk15UfY0UhfHU90eMIcRYJgwmFVsawBMZzzwZAjlogxHHoI5olQTwjiYHsCGILwyKovkjS%2BH4KSYJlKN%2BST/jrZAmJYzA2IgTTTgAMSOFIQEBa0iWiMyuXHCyqw4FZaE4fxeD8DgtFIVBOGpSxfQUNYNnpXYeFIAhNCClYAGsEkCEKOEkcLMuizheAUEANHSzKVjgWAkDQFgUjoOJyEoRrmvoeIDkMYAuA0AbSCwAdZMwAA1PBMAAdwAeVPCK0poWgCDiCqIGiErojCJoE04NLGrYQQZoYWhdsi3gsBEoxxHOoa8Cubohwq27MFULpsS2NKhJqEraDwaImOIBMPCwErtLwFg9qCvgDGABQJumubGChmRBBEMR2CkVH5CUNQSt0Lh9F6lBrGsfR/oqyAVhPOpSU4J0QSvUx4ssLhEiOJ0Zp2cqai6WmXAYdxPDaPQQjmMoKj0NIMlpyY/EJ6WigYAYJeGQnOm6eoZjlvQNdp3pmhVoZ4nV7XhbyU2%2BiNhYTZWRL1k2CRgtC4rbpijgjlUAAOM0nTNSRjQMEj%2Bs9DRQ6OCBcEIEhCR2Lgll4DLzqWHK8v0TgitICKovd8rKuq5PSDqxAQAlFJsTaiAOpa4gIjpThvd9/3A96o4Q7DqLMHwC96L0fg0dEcQsf7nGVHUW6CdIKamPc7hoYKsKs5K92ZuxcucVQKhPZ9v2A564OO/DhtUCamvY/jxOatTsxJE9ABOB/H6fp%2BzXTwrXZzsrbHzpOtFqmAS6vXeheSuTQ4bKEMDUIQCBUBTQWrwauiksgQPCLQaBsDs4IJPp1YYcNmApAUDAggpBEFxFXsIGBcCSpAOQA8YgcMv40IaPgCKvAR7oyHtIEeigx74z0EHYwpNLDk2iJTYU0U2JZGepzbmqwHaY1sG%2BMIKCoGUPgelK4n1eDTyYLPHgzsOCL0waVDg2A3rIEPMQbeTcA7ABYhHbSXgGDZThA2IRNgjhRwvOfBOBc/7X3yhnD%2BvBc7fyqr/LKb8zDBJMZfZOKwhzEAyM4SQQA

[Febbe]

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.

Can you post preferably a minimal reproducible example?
Alternatively, you could write me again the exact position of the false positive. Including the implicit conversion operator.

[firewave]

In D137205#3936944, @firewave wrote:

I still have another false positive with static variables but have not gotten around reducing it yet.

Those false positives no longer appear. So no more open FPs from my side. Great work!

[njames93]

@sammccall I have a feeling you're gonna want to examine this checks feasibility in clangd.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

80

We generally avoid trailing return types.

97

The same rules for auto apply here, this should be explicit about what pointer it's returning.

124

Matching over the entire context seems pretty and a huge drain on performance, would it not make sense to just match inside the function declaration.
Side note maybe a RecursiveASTVisitor would make more sense here in terms of performance.

Same goes for isInLambdaCapture.

291

What's the reason for this logic, they require a different fix -
x => x(std::move(x))
And a checking langopts for c++14.

Or is this because of implicit captures?

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h

31

We typically avoid defining all the special member functions for clang tidy checks. The destructor typically only needs to be defined if it's definition can't be inside the header file.

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

2

Running this check explicitly in c++11/17 implies you expect different diagnostics, if so you can use the --check-suffixes flag to enable checking configurations. Search for other tests which use that if you're unsure

17

This struct appears to be unused and has exactly the same definition as Movable.

[Febbe]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

80

Ok, I'll make them non-trailing.
Personally, I find trailing return types more readable, and they always work like expected.

124

How can I reduce the Context?

Also, in which terms, the RecursiveASTVisitor would make more sense here?

291

Yes, all fixes require at least c++14.
Also, implicit captures, require a different fix.

So this is for both.
Honestly, I was too lazy to also implement some sort of lambda capture builder, which handles all of this.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h

31

It can't be, because I forward declared CFG, to reduce compile time a bit.
If you prefer to include the CFG's definition in the header, I can do that.

Shall I still remove the deleted special member functions?

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

17

No, Movable is not trivially capyable, because it does not define a default destructor, Copyable does.

[Febbe]

Improved fixit suggestion.

• No duplicated replacements.

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

[sammccall]

In D137205#3939720, @njames93 wrote:

@sammccall I have a feeling you're gonna want to examine this checks feasibility in clangd.

Thanks! As it uses the CFG, by default we're going to have to turn it off (AFAIK building the CFG with broken code can still crash).
If you don't mind, please add it to the exclude list in clang-tools-extra/clangd/TidyProvider.cpp next to -bugprone-use-after-move (or I can do this after it lands).

[Skylion007]

Typo

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

228

Typo: IsMoveAssignable

[Febbe]

Replaces match clauses with RecursiveASTVisistors

• doubled performance
• fixed also a bug in template spezialisations

[Febbe]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

124

Replaced with RecursiveASTVisitor.

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

2

Not necessarily, I just want the check to work with c++11 and after the changes to the language after c++17 (e.g. guaranteed copy elision).

[kadircet]

clang-tools-extra/clangd/TidyProvider.cpp

215

comment seems half-finished here. is there any other reason than trying to analyze CFG for invalid code?

[kadircet]

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.

[Febbe]

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.

I think this should be regulated / enforced via clang-format if it is relevant at all to somebody.
In terms of consistency, the east-const I use is more consistent. Not to the previous written code, but to the language.
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.

In my eyes it should therefore always be east-const and also be taught in such a way, since such irritations do not arise thereby at all.

When it is ok for you to keep the east-const I would like to leave it as is, because it is on top a lot of work to manually search and replace those occurrences.

[Febbe]

Fixed typo

[Febbe]

clang-tools-extra/clangd/TidyProvider.cpp

215

Thanks, the "and" does not belong here.

[kadircet]

In D137205#3951230, @Febbe wrote:

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.

I think this should be regulated / enforced via clang-format if it is relevant at all to somebody.

surely, it would be nice to have a clang-tidy check or something (by design clang-format only performs white-space changes).

In terms of consistency, the east-const I use is more consistent. Not to the previous written code, but to the language.

not sure what you mean by it's consistent "to the language", but right now even in your header files you have both east and west consts. probably to keep public interfaces consistent across the codebase, as you've noticed.
another consequence of this is anyone modifying this code later on, will need to make a choice between sticking to the code inside this file vs code inside the rest of the codebase, which will create more discussions with every patch making people lose time and moreover bring the file into an inconsistent state, as some of those discussions will insert west consts.

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.

In my eyes it should therefore always be east-const and also be taught in such a way, since such irritations do not arise thereby at all.

When it is ok for you to keep the east-const I would like to leave it as is, because it is on top a lot of work to manually search and replace those occurrences.

As mentioned above, I understand your opinion here and deep down wish that was the state. But as I explained before, in a code base where thousands of developers are contributing, I don't think we can afford everyone introducing code with their own opinions. Despite not explicitly having a style guide on east vs west consts, it's clear from the rest of the codebase that one should adhere to west consts.
So I'd be glad if you could take your time to replace east consts to west.

[carlosgalvezp]

by design clang-format only performs white-space changes

clang-format does support east/west const enforcement with the QualifierAlignment option. From experience, I strongly encourage repo owners to enable it repo-wide to avoid these kinds of discussions. Until then, I believe developers should keep consistency with the existing style in the codebase, regardless of personal views. This goes in line with the LLVM Coding Standards:

"If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow."

[sammccall]

In D137205#3952864, @carlosgalvezp wrote:

by design clang-format only performs white-space changes

clang-format does support east/west const enforcement with the QualifierAlignment option. From experience, I strongly encourage repo owners to enable it repo-wide to avoid these kinds of discussions.

FWIW, one reason this isn't enabled is that it's relatively recent and also not completely safe in principle.
It's not as simple as "if LLVM cares about const-alignment, this flag should be on".

(And I don't think @carlosgalvezp was saying it was that simple! I'm just saying we can't rehash the discussion here because it's too big - maybe we should elsewhere).

Until then, I believe developers should keep consistency with the existing style in the codebase, regardless of personal views. This goes in line with the LLVM Coding Standards:

+1

[carlosgalvezp]

we can't rehash the discussion here because it's too big

Indeed, it's a major task to undertake so I don't mean to hijack this thread with that :) Just wanted to point out clang-format supports it in case it's of interest (it's a fairly new addition that not everyone might be aware of).

[Febbe]

Applied clang-format QualifierAlignment: Left

[Febbe]

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.

Exactly this.

surely, it would be nice to have a clang-tidy check or something (by design clang-format only performs white-space changes).

It would also be good to have some sort of refactoring choice in clangd to swap the alignment.
Maybe we should add a feature request on GitHub for both.

FWIW, one reason this isn't enabled is that it's relatively recent and also not completely safe in principle.

It's not as simple as "if LLVM cares about const-alignment, this flag should be on".

I also was curious about that, until I've read the documentation about this formatter option. But is it safe for already correct alignments?
Then it could be used in the linting step to just verify, that it is done correctly, am I right?

Back to topic

I have some open questions:

@njames93 asked me to remove the explicitly deleted copy/move constructors/assignment operators.
But this would break the rule of 5, or I have to move the CFG.h include into the header which is in my opinion also against the coding convention.
Is it ok to keep them, should I break the rule of 5 or should I move the CFG.h include into the header?

@njames93 also mentioned, that the complete traversal of the Context is a waste of resources.
But I have no clue, how to use the Context down from the top function.
Is there even a way to do so, or do I have to start with TraverseFunctionDecl in the RecursiveASTVisitor?

An even better approach would be to start with the DeclRefExpr itself.
Is there a way to traverse a matcher in the reverse order from the DeclRefExpr as start?
Or do I have to parse it by hand then?

Currently, I got many reviews regarding the coding stile and the result, but I would also like to see a review to the CFG parsing itself.

There are also "open improvements", but they require a huge amount of work on top of this patch:

• fixups in lambda captures:
  • To fixup lambda captures sustainably, like the handling of multiple last usages in the capture list / implicit capture. The new capture list must be built at the end of the parsing of the translation unit.
  • Some sort of capture-list builder must be used, which first collects all last usages, and then creates one single fix up for all.
  • In my opinion, this is non-blocking since the benefit is small compared to the work.
  • I am willing to add this, if desired, because I have a plan how I would implement it.
• A heuristic to catch copyable but non trivially copyable and movable "guards".
  • This is a lot harder, and I actually have no clue how to do this without scanning each type recursively for side effects to child fields in any destructor which are not directly followed by memory management (destruction).
  • I would like to defer this.
• There is no detection of last usages of fields
  • This is also non-blocking, since it is a pure feature.

[Skylion007]

Pinging @njames93 to about the questions from the previous comment. Would love to see something like this PR get merged.

[ccotter]

@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,

• Should we detect "escaped" values, i.e., values who have their address taken, and not apply a FixIt in these cases? See this test.
• I took a slightly more conservative approach in only moving values whose QualType.isTrivialType() is true, which addresses the shared_ptr<lock_guard> problem. We can then apply FixIts for containers (std::vector etc) and algebraic types (tuple/optional) of said types (and containers of containers of ...). In general, the approach I was aiming for was to define a classification of "safe to move types" (inferred from isTrivialType, and/or using a list of safe types of in the tool configuration as you have done). Containers/algebraic types of safe to move types, or smart pointers of safe to move types (being careful with any custom allocator which might lead to a similar problem as the scope guard case) are also safe to move, etc. Having the tool be able to distinguish "safe to move" FixIts from "potentially unsafe" (as a tool mode, potentially with "safe" as the default) would ensure safe refactoring.
• This might be a nitpick, but in the diagnostic message warning: Parameter '...', "Parameter" typically means something more specific (the names given to function variables, aka, function parameters). How about warning: Value '...'

Would you be interested in collaborating on this? I'm not sure the status of when this phab would land, but I could contribute my suggestions after, or into, this phab depending on feedback. I wrote up a few more notes on https://discourse.llvm.org/t/new-clang-tidy-check-finding-move-candidates/67252 (again, before I realized that you had started this work already).

[Febbe]

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,

Hello @ccotter cool, you found this phab, and that you also had the same idea.
I would like it, to collaborate with you. I just don't have a clue how to do this properly and clean via phabricator. What's your suggestion?

• Should we detect "escaped" values, i.e., values who have their address taken, and not apply a FixIt in these cases? See this test.

Currently, this check does not care, if a value is taken by reference or pointer, not even when the value is taken in the same scope.
This is in fact an open Todo.
A rule of thumb would be: if the called function is analyzable, analyze it. Else, declare the move safety as unsecure and do warn, if the user is interested in it.

Generally I think, that fix it's should be always displayed, when they produce compilable code. But the move safety should be displayed in the error message.
Of course, it would be good to disable questionable fixups for automatic fix up runs. But I didn't heart of a solution to differ in a clang-tidy check, between automatic fixups and those used in an IDE.

• I took a slightly more conservative approach in only moving values whose QualType.isTrivialType() is true, which addresses the shared_ptr<lock_guard> problem. We can then apply FixIts for containers (std::vector etc) and algebraic types (tuple/optional) of said types (and containers of containers of ...). In general, the approach I was aiming for was to define a classification of "safe to move types" (inferred from isTrivialType, and/or using a list of safe types of in the tool configuration as you have done). Containers/algebraic types of safe to move types, or smart pointers of safe to move types (being careful with any custom allocator which might lead to a similar problem as the scope guard case) are also safe to move, etc. Having the tool be able to distinguish "safe to move" FixIts from "potentially unsafe" (as a tool mode, potentially with "safe" as the default) would ensure safe refactoring.

But moving trivial types are equal to a copy, aren't they? Therefore, I search for types where this is false.
I also think, because clang-tidy does not claim to be 100% correct, in contrast to a compiler which must be, having such cases like shared_ptr<lock_guard> is acceptable. Mostly such cases also have a huge smell (store mutex along with the data etc...).

• This might be a nitpick, but in the diagnostic message warning: Parameter '...', "Parameter" typically means something more specific (the names given to function variables, aka, function parameters). How about warning: Value '...'

Thank you for the catch.

[Skylion007]

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.

[Febbe]

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.

Does this only happen on "init-statements" of while/if/switch?
And thanks for the catch, I'll take a look into it as soon I have more time.

Btw. I've oriented myself at the "bugprone-use-after-move" check, but simply inverting it does not work here.

[Skylion007]

In D137205#4054534, @Febbe wrote:

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.

Does this only happen on "init-statements" of while/if/switch?
And thanks for the catch, I'll take a look into it as soon I have more time.

Btw. I've oriented myself at the "bugprone-use-after-move" check, but simply inverting it does not work here.

It happens in any conditional statements.

Another common failure case

while(shouldStop(std::move(a))){
    // doSomething
    // a is last usage, but shouldStop called multiple times
}

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

77

Could you add another test where Mov is just a Movable value?

[ccotter]

@Febbe - checking in. is this still on your radar? I would love to see this merged!

[firewave]

Some additional thoughts I had a while ago about something I have raised before:
I think the warnings which can only be fixed with c++14 should either only be issued if that standard was specified or be behind an option. We have lots of lambda captures which could be moved with c++14 and having those warnings would lead to lots of suppressions within the code since we only target c++11.

[Febbe]

In D137205#4222920, @ccotter wrote:

@Febbe - checking in. is this still on your radar? I would love to see this merged!

Yes, this is still on my radar, but currently, I am not satisfied with my solution. There are too many issues, and I may have to rewrite/fix the whole search algorithm.
The last three months I was too busy with both university and my job.

[Febbe]

In D137205#4224236, @firewave wrote:

Some additional thoughts I had a while ago about something I have raised before:
I think the warnings which can only be fixed with c++14 should either only be issued if that standard was specified or be behind an option. We have lots of lambda captures which could be moved with c++14 and having those warnings would lead to lots of suppressions within the code since we only target c++11.

Yes, I agree, while I can't understand, why someone still wants to use only c++11 I can totally understand, that a single Software Engineer can't do anything about this, when the corporation does not permit it.
But it should not be removed, since this warning can still show some pitfalls in performance critical code. A way in c++11 to fix the warning, is to create a functor, instead of a lambda.

[firewave]

In D137205#4225006, @Febbe wrote:

Yes, I agree, while I can't understand, why someone still wants to use only c++11 I can totally understand, that a single Software Engineer can't do anything about this, when the corporation does not permit it.
But it should not be removed, since this warning can still show some pitfalls in performance critical code. A way in c++11 to fix the warning, is to create a functor, instead of a lambda.

Well, compatibility with older platforms. And I personally have lots of quarrels with modern (and more recently even early) C++, but let's not get into that. Being able to move within captures is not one of them though...

I still think being able to tune that check for certain parts would be helpful - especially if it would not be fixable with a fix-it. It should also help with applying these findings to an existing code base as you could enable it incrementally (especially if you need to introduce something like functors).
Recent Clang additions like misc-const-correctness or -Wunsafe-buffer-usage would have profited from being just a bit more granular as they produce such a flood of warnings even for small code bases which makes it quite troublesome to adopt the code for those warnings as some of them are not just applying the fix-it but also need to be looked at in the bigger picture to see if they might not impact things negatively in the future.

[PiotrZSL]

To be honest, i got this check implemented and running already on some big code base since years, and inserted few thousands of std::move's with 0 false-positives, maybe not finding all cases, but I'm satisfied.
I will put here source code, maybe you will get some ideas, but still I originally implemented it on Clang 5, so code seen some evolution, and now probably there are better ways to do these things. You don't need any magical heuristic or algorithms.

MissingStdMove.cpp25 KBDownload

clang-tools-extra/clang-tidy/performance/CMakeLists.txt

add_clang_library(clangTidyPerformanceModule
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp
   NoAutomaticMoveCheck.cpp
   NoIntToPtrCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   PerformanceTidyModule.cpp
   TriviallyDestructibleCheck.cpp
   TypePromotionInMathFnCheck.cpp
+  UnnecessaryCopyOnLastUseCheck.cpp
   UnnecessaryCopyInitialization.cpp
   UnnecessaryValueParamCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils
 
   DEPENDS

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoAutomaticMoveCheck.h"
 #include "NoIntToPtrCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "TriviallyDestructibleCheck.h"
 #include "TypePromotionInMathFnCheck.h"
 #include "UnnecessaryCopyInitialization.h"
+#include "UnnecessaryCopyOnLastUseCheck.h"
 #include "UnnecessaryValueParamCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "performance-noexcept-move-constructor");
     CheckFactories.registerCheck<TriviallyDestructibleCheck>(
         "performance-trivially-destructible");
     CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
         "performance-type-promotion-in-math-fn");
     CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
         "performance-unnecessary-copy-initialization");
+    CheckFactories.registerCheck<UnnecessaryCopyOnLastUseCheck>(
+        "performance-unnecessary-copy-on-last-use");
     CheckFactories.registerCheck<UnnecessaryValueParamCheck>(
         "performance-unnecessary-value-param");
   }
 };
 
 // Register the PerformanceModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<PerformanceModule>
     X("performance-module", "Adds performance checks.");

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h

+//===--- UnnecessaryCopyOnLastUseCheck.h - clang-tidy ------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
+#include "llvm/ADT/DenseMap.h"
+
+namespace clang {
+
+class CFG;
+namespace tidy::performance {
+
+/// A check that flags value parameters on last usages that can safely be moved,
+/// because they are copied anyway.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-copy-on-last-use.html
+
+class UnnecessaryCopyOnLastUseCheck : public ClangTidyCheck {
+public:
+  UnnecessaryCopyOnLastUseCheck(StringRef Name, ClangTidyContext *Context);
+  UnnecessaryCopyOnLastUseCheck(UnnecessaryCopyOnLastUseCheck &&) = delete;
+  UnnecessaryCopyOnLastUseCheck(const UnnecessaryCopyOnLastUseCheck &) = delete;
+  UnnecessaryCopyOnLastUseCheck &
+  operator=(UnnecessaryCopyOnLastUseCheck &&) = delete;
+  UnnecessaryCopyOnLastUseCheck &
+  operator=(const UnnecessaryCopyOnLastUseCheck &) = delete;
+  ~UnnecessaryCopyOnLastUseCheck() override;
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void onEndOfTranslationUnit() override;
+
+private:
+  CFG *getOrCreateCFG(const FunctionDecl *FD, ASTContext *C);
+
+  utils::IncludeInserter Inserter;
+  const std::vector<StringRef> BlockedTypes;
+  const std::vector<StringRef> BlockedFunctions;
+
+  // cfg cache
+  llvm::DenseMap<const FunctionDecl *, std::unique_ptr<CFG>> CFGs;
+};
+
+} // namespace tidy::performance
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp

+//===--- UnnecessaryCopyOnLastUseCheck.cpp - clang-tidy -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnnecessaryCopyOnLastUseCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include <algorithm>
+#include <cassert>
+#include <cstddef>
+#include <utility>
+
+using namespace clang;
+using namespace clang::tidy;
+using namespace clang::tidy::performance;
+using namespace clang::ast_matchers;
+
+static constexpr auto BlockedTypesOption = "BlockedTypes";
+static constexpr auto BlockedFunctionsOption = "BlockedFunctions";
+
+namespace {
+struct FindDeclRefBlockReturn {
+  const CFGBlock *DeclRefBlock = nullptr;
+  CFGBlock::const_iterator StartElement{};
+};
+
+enum class Usage {
+  Error = -1,
+  Usage = 0,
+  DefiniteLastUse,
+  LikelyDefiniteLastUse,
+};
+
+} // namespace
+
+static FindDeclRefBlockReturn findDeclRefBlock(CFG const *TheCFG,
+                                               const DeclRefExpr *DeclRef) {
+  for (CFGBlock *Block : *TheCFG) {
+    auto Iter =
+        llvm::find_if(Block->Elements, [&, DeclRef](const CFGElement &Element) {
+          if (Element.getKind() == CFGElement::Statement) {
+            return Element.template castAs<CFGStmt>().getStmt() == DeclRef;
+          }
+          return false;
+        });
+    if (Iter != Block->Elements.end()) {
+      return {Block, ++Iter};
+    }
+  }
+  return {nullptr, {}};
+}
+
+static const clang::CFGElement *
+nextUsageInCurrentBlock(const FindDeclRefBlockReturn &StartBlockElement,
+                        const DeclRefExpr *DeclRef) {
+  // Search for uses in the current block
+  auto Begi = StartBlockElement.StartElement;
+  auto Endi = StartBlockElement.DeclRefBlock->Elements.end();
+  auto Iter = std::find_if(Begi, Endi, [&](const CFGElement &Element) {
+    if (Element.getKind() == CFGElement::Statement) {
+      if (auto *Stmt = Element.template castAs<CFGStmt>().getStmt()) {
+        if (auto *DRE = dyn_cast<DeclRefExpr>(Stmt)) {
+          if (DRE->getDecl() == DeclRef->getDecl()) {
+            return true;
+          }
+        }
+      }
+    }
+    return false;
+  });
+  return Iter != Endi ? &*Iter : nullptr;
+}
+
+static bool isLHSOfAssignment(const DeclRefExpr *DeclRef, ASTContext &Context) {
+  const TraversalKindScope RAII(Context, TK_IgnoreUnlessSpelledInSource);
+  // Todo (performance): While this is faster than a match expression,
+  //      it would be faster to start from the DeclRefExpr directly
+  struct IsLHSOfAssignment : RecursiveASTVisitor<IsLHSOfAssignment> {
+    const DeclRefExpr *Ref{};
+
+    IsLHSOfAssignment(const DeclRefExpr *Ref, ASTContext &Context) : Ref(Ref) {}
+
+    bool shouldWalkTypesOfTypeLocs() const { return false; }
+    bool shouldVisitTemplateInstantiations() const { return true; }
+
+    bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *BO) {
+      if (BO->isAssignmentOp()) {
+        if (auto *DRE =
+                dyn_cast<DeclRefExpr>(BO->getArg(0)->IgnoreParenImpCasts())) {
+          if (DRE && DRE == Ref) {
+            return false;
+          }
+        }
+      }
+      return true;
+    }
+  };
+  return !IsLHSOfAssignment{DeclRef, Context}.TraverseAST(Context);
+}
+
+static bool isInLambdaCapture(const DeclRefExpr *MyDeclRef,
+                              ASTContext &Context) {
+
+  // Todo (improvement): Expand this Visitor to also determine, if an explicit
+  // CaptureInitExpr is used or if a DeclRefExpr is used implicitelly via a
+  // cature default
+  struct IsInLambdaCapture : RecursiveASTVisitor<IsInLambdaCapture> {
+    const DeclRefExpr *Ref{};
+
+    IsInLambdaCapture(const DeclRefExpr *Ref) : Ref(Ref) {}
+
+    bool shouldWalkTypesOfTypeLocs() const { return false; }
+    bool shouldVisitTemplateInstantiations() const { return true; }
+
+    bool VisitLambdaExpr(LambdaExpr *Lambda) {
+      for (Expr *Inits : Lambda->capture_inits()) {
+        auto *S = cast<Stmt>(Inits);
+        llvm::SmallVector<Stmt *, 6> Childs;
+        while (S != nullptr) {
+          if (auto *DeclRef = dyn_cast<DeclRefExpr>(S);
+              DeclRef != nullptr && Ref == DeclRef) {
+            return false;
+          }
+          Childs.append(S->child_begin(), S->child_end());
+          S = Childs.empty() ? nullptr : Childs.pop_back_val();
+        }
+      }
+      return true;
+    }
+  };
+  return !IsInLambdaCapture{MyDeclRef}.TraverseAST(Context);
+}
+
+static Usage definiteLastUse(ASTContext *Context, CFG *const TheCFG,
+                             const DeclRefExpr *DeclRef) {
+  if (TheCFG == nullptr) {
+    return Usage::Error;
+  }
+
+  // Find the CFGBlock containing the DeclRefExpr
+  auto StartBlockElement = findDeclRefBlock(TheCFG, DeclRef);
+  if (StartBlockElement.DeclRefBlock == nullptr) {
+    return Usage::Error; // Todo(unexpected): DeclRefExpr not found in CFG
+  }
+
+  // Find next uses of the DeclRefExpr
+
+  auto TraverseCFGForUsage = [&]() -> Usage {
+    llvm::SmallPtrSet<CFGBlock const *, 8> VisitedBlocks;
+    llvm::SmallVector<CFGBlock const *, 8> Worklist;
+
+    auto HandleInternal = [&](FindDeclRefBlockReturn const &BlockElement) {
+      CFGElement const *NextUsageE =
+          nextUsageInCurrentBlock(BlockElement, DeclRef);
+      if (NextUsageE) {
+        if (bool const IsLastUsage =
+                isLHSOfAssignment(llvm::cast<DeclRefExpr>(
+                                      NextUsageE->castAs<CFGStmt>().getStmt()),
+                                  *Context);
+            !IsLastUsage) {
+          return Usage::Usage;
+        }
+        return Usage::DefiniteLastUse;
+      }
+      assert(BlockElement.DeclRefBlock);
+      // No successing DeclRefExpr found, appending successors
+      for (CFGBlock const *Succ : BlockElement.DeclRefBlock->succs()) {
+        if (Succ) { // Succ can be nullptr, if a block is unreachable
+          Worklist.push_back(Succ);
+        }
+      }
+      return Usage::DefiniteLastUse; // No usage found, assume last use
+    };
+
+    if (auto FoundUsage = HandleInternal(StartBlockElement);
+        FoundUsage == Usage::Usage) { // Usage found
+      return FoundUsage;
+    }
+    while (!Worklist.empty()) {
+      CFGBlock const *Block = Worklist.pop_back_val();
+      if (!VisitedBlocks.insert(Block).second) {
+        continue;
+      }
+      if (auto FoundUsage = HandleInternal({Block, Block->Elements.begin()});
+          FoundUsage == Usage::Usage) {
+        return FoundUsage;
+      }
+    }
+    return Usage::DefiniteLastUse;
+  };
+
+  return TraverseCFGForUsage();
+}
+
+namespace clang::tidy::performance {
+
+UnnecessaryCopyOnLastUseCheck::~UnnecessaryCopyOnLastUseCheck() = default;
+
+UnnecessaryCopyOnLastUseCheck::UnnecessaryCopyOnLastUseCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      Inserter(Options.getLocalOrGlobal("IncludeStyle",
+                                        utils::IncludeSorter::IS_LLVM),
+               areDiagsSelfContained()),
+      BlockedTypes(
+          utils::options::parseStringList(Options.get(BlockedTypesOption, ""))),
+      BlockedFunctions(utils::options::parseStringList(
+          Options.get(BlockedFunctionsOption, ""))),
+      CFGs() {}
+
+void UnnecessaryCopyOnLastUseCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ValueParameter =
+      declRefExpr(
+          to(valueDecl(
+              unless(varDecl(unless(hasAutomaticStorageDuration()))),
+              hasType(qualType(
+                  hasCanonicalType(qualType(
+                      matchers::isExpensiveToCopy(),
+                      unless(anyOf(isConstQualified(), lValueReferenceType(),
+                                   pointerType())))),
+                  unless(hasDeclaration(namedDecl(
+                      matchers::matchesAnyListedName(BlockedTypes))) //
+                         ))))))
+          .bind("param");
+
+  const auto IsMoveAssignable = cxxOperatorCallExpr(
+      hasDeclaration(cxxMethodDecl(
+          isCopyAssignmentOperator(),
+          ofClass(hasMethod(cxxMethodDecl(isMoveAssignmentOperator(),
+                                          unless(isDeleted())))))),
+      hasRHS(ValueParameter));
+
+  const auto IsMoveConstructible =
+      ignoringElidableConstructorCall(ignoringParenImpCasts(
+          cxxConstructExpr(
+              unless(hasParent(callExpr(hasDeclaration(namedDecl(
+                  matchers::matchesAnyListedName(BlockedFunctions)))))),
+              hasDeclaration(cxxConstructorDecl(
+                  isCopyConstructor(),
+                  ofClass(hasMethod(cxxConstructorDecl(isMoveConstructor(),
+                                                       unless(isDeleted())))))),
+              hasArgument(0, ValueParameter))
+              .bind("constructExpr")));
+
+  Finder->addMatcher(stmt(anyOf(IsMoveAssignable, expr(IsMoveConstructible))),
+                     this);
+}
+
+void UnnecessaryCopyOnLastUseCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Param = Result.Nodes.getNodeAs<DeclRefExpr>("param");
+  const ValueDecl *const DeclOfParam = Param->getDecl();
+  const DeclContext *const FunctionOfDeclContext =
+      DeclOfParam->getParentFunctionOrMethod();
+
+  if (!FunctionOfDeclContext) {
+    // The parameter is not defined in a function, therefore it is not
+    // possible to check if it is the last use via CFG analysis
+    // Todo (improvement): Add a flag to show unanalyzable cases
+    return;
+  }
+
+  const auto *const FunctionOfDecl =
+      llvm::cast<FunctionDecl>(FunctionOfDeclContext);
+
+  const auto *const VarDeclVal = llvm::dyn_cast<VarDecl>(DeclOfParam);
+  if (!VarDeclVal) {
+    return;
+  }
+
+  Usage DefiniteLastUse = definiteLastUse(
+      Result.Context, getOrCreateCFG(FunctionOfDecl, Result.Context), Param);
+
+  if (DefiniteLastUse == Usage::Usage || DefiniteLastUse == Usage::Error) {
+    return;
+  }
+
+  // Template code cant be fixed currently
+  if (!FunctionOfDecl->isTemplateInstantiation()) {
+    clang::SourceManager &SM = *Result.SourceManager;
+    auto Diag =
+        diag(
+            Param->getExprLoc(),
+            "Parameter '%0' is copied on last use, consider moving it instead.")
+        << Param->getDecl()->getNameAsString();
+
+    if (auto *CExpr = Result.Nodes.getNodeAs<CXXConstructExpr>("constructExpr");
+        isInLambdaCapture(Param, *Result.Context) ||
+        (CExpr && CExpr->getExprLoc().isMacroID())) {
+      // Lambda captures should not be fixed.
+      // They also require at least c++14
+      return;
+    }
+    auto MVStmt = "std::move(" + Param->getDecl()->getNameAsString() + ")";
+    Diag << FixItHint::CreateReplacement(Param->getSourceRange(), MVStmt)
+         << Param->getDecl()->getNameAsString()
+         << Inserter.createIncludeInsertion(SM.getFileID(Param->getBeginLoc()),
+                                            "<utility>");
+  } else { // Template code can't be fixed currently, also a std::forward may be
+           // more appropriate
+    auto Diag =
+        diag(Param->getExprLoc(), "Parameter '%0' may be copied on last use, "
+                                  "consider forwarding it instead.")
+        << Param->getDecl()->getNameAsString();
+  }
+}
+
+void UnnecessaryCopyOnLastUseCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  Inserter.registerPreprocessor(PP);
+}
+
+void UnnecessaryCopyOnLastUseCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle", Inserter.getStyle());
+  Options.store(Opts, BlockedTypesOption,
+                utils::options::serializeStringList(BlockedTypes));
+  Options.store(Opts, BlockedFunctionsOption,
+                utils::options::serializeStringList(BlockedFunctions));
+}
+
+void UnnecessaryCopyOnLastUseCheck::onEndOfTranslationUnit() {}
+
+static CFG::BuildOptions createBuildOptions() {
+  CFG::BuildOptions Options;
+  Options.setAlwaysAdd(DeclRefExpr::DeclRefExprClass);
+  Options.AddImplicitDtors = true;
+  Options.AddTemporaryDtors = true;
+  return Options;
+}
+
+CFG *UnnecessaryCopyOnLastUseCheck::getOrCreateCFG(const FunctionDecl *FD,
+                                                   ASTContext *C) {
+  static auto BO = createBuildOptions();
+  if (auto Iter = this->CFGs.find(FD); Iter != this->CFGs.end()) {
+    return Iter->second.get();
+  }
+
+  auto EmplaceResult =
+      this->CFGs.try_emplace(FD, CFG::buildCFG(nullptr, FD->getBody(), C, BO));
+  return EmplaceResult.first->second.get();
+}
+} // namespace clang::tidy::performance

clang-tools-extra/clangd/TidyProvider.cpp

static const std::string BadChecks =
                        "-llvm-header-guard",
 
                        // ----- Crashing Checks -----
 
                        // Check can choke on invalid (intermediate) c++
                        // code, which is often the case when clangd
                        // tries to build an AST.
                        "-bugprone-use-after-move",
+                       // Using an CFG might crash on invalid code:
+                       "-performance-unnecessary-copy-on-last-use",
                        // Alias for bugprone-use-after-move.
                        "-hicpp-invalid-access-moved",
 
                        // ----- Performance problems -----
 
                        // This check runs expensive analysis for each variable.
                        // It has been observed to increase reparse time by 10x.
                        "-misc-const-correctness");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 - New :doc:`cppcoreguidelines-avoid-do-while
   <clang-tidy/checks/cppcoreguidelines/avoid-do-while>` check.
 
   Warns when using ``do-while`` loops.
 
+- New :doc:`performance-unnecessary-copy-on-last-use
+  <clang-tidy/checks/performance/unnecessary-copy-on-last-use>` check.
+
+  Finds variables of non-trivially-copyable types, that are used in a copy
+  construction on their last usage and suggest to wrap the usage in a
+  ``std::move``.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `performance-move-const-arg <performance/move-const-arg.html>`_, "Yes"
    `performance-move-constructor-init <performance/move-constructor-init.html>`_,
    `performance-no-automatic-move <performance/no-automatic-move.html>`_,
    `performance-no-int-to-ptr <performance/no-int-to-ptr.html>`_,
    `performance-noexcept-move-constructor <performance/noexcept-move-constructor.html>`_, "Yes"
    `performance-trivially-destructible <performance/trivially-destructible.html>`_, "Yes"
    `performance-type-promotion-in-math-fn <performance/type-promotion-in-math-fn.html>`_, "Yes"
    `performance-unnecessary-copy-initialization <performance/unnecessary-copy-initialization.html>`_, "Yes"
+   `performance-unnecessary-copy-on-last-use <performance/unnecessary-copy-on-last-use.html>`_, "Yes"
    `performance-unnecessary-value-param <performance/unnecessary-value-param.html>`_, "Yes"
    `portability-restrict-system-includes <portability/restrict-system-includes.html>`_, "Yes"
    `portability-simd-intrinsics <portability/simd-intrinsics.html>`_,
    `portability-std-allocator-const <portability/std-allocator-const.html>`_,
    `readability-avoid-const-params-in-decls <readability/avoid-const-params-in-decls.html>`_, "Yes"
    `readability-braces-around-statements <readability/braces-around-statements.html>`_, "Yes"
    `readability-const-return-type <readability/const-return-type.html>`_, "Yes"
    `readability-container-contains <readability/container-contains.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst

+.. title:: clang-tidy - performance-unnecessary-copy-on-last-use
+
+performance-unnecessary-copy-on-last-use
+========================================
+
+Finds variables of non-trivially-copyable types, that are used in a copy
+construction on their last usage and suggest to wrap the usage in a
+``std::move``. The usage just before an assignment is interpreted as last usage.
+
+Many programmers tend to forget ``std::move`` for variables that can be moved.
+Instead, the compiler creates implicit copies, which can significantly decrease
+performance.
+
+Example
+--------
+
+.. code-block:: c++
+
+  void acceptor(std::vector<int> v);
+  void Function() {
+    std::vector<int> v;
+    v.emplace_back(20);
+    // The warning will suggest passing this as a rvalue-reference
+    acceptor(v);
+  }
+
+Options
+-------
+
+.. option:: BlockedTypes
+
+   A semicolon-separated list of names of types allowed to be copied on last
+   usage. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
+   every type with suffix `Ref`, `ref`, `Reference` and `reference`.
+   The default is empty. If a name in the list contains the sequence `::` it
+   is matched against the qualified typename (i.e. `namespace::Type`,
+   otherwise it is matched against only the type name (i.e. `Type`).
+
+.. option:: BlockedFunctions
+
+   A semicolon-separated list of names of functions who's parameters do not
+   participate in the resolution.
+   Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
+   every type with suffix `Ref`, `ref`, `Reference` and `reference`.
+   The default is empty. If a name in the list contains the sequence `::` it
+   is matched against the qualified typename (i.e. `namespace::Type`,
+   otherwise it is matched against only the type name (i.e. `Type`).
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+Limitations
+-----------
+
+This check does not implement a heuristic, if a variable is used as guard until
+the end of it's scope. It also does not check, if a variable has any side 
+effects in the destructor, which must be applied at the end of the scope.
+Therefore, it will suggest to use ``std::move`` in the
+following case, where `guard` protects `i`:
+
+.. code-block:: cpp
+    
+  void acceptor(std::shared_ptr<std::unique_lock<std::mutex>>, int& i);
+
+  void f(std::shared_ptr<std::unique_lock<std::mutex>> guard, int& i) {
+    acceptor(guard, i);
+    i++;
+  }
+
+This check is also unable to detect last usages for fields, if the parent 
+class is destructed after the last usage.
+
+Implicit copies in lambda-captures are detected, but no fixit is provided.
+Also, the check will warn for c++11 even if it is not possible to fix it without
+an language upgrade to at least c++14.

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp

+// RUN: %check_clang_tidy %s -std=c++17 performance-unnecessary-copy-on-last-use %t
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+// CHECK-FIXES: #include <utility>
+
+struct Movable {
+  Movable() = default;
+  Movable(Movable const &) = default;
+  Movable(Movable &&) = default;
+  Movable &operator=(Movable const &) = default;
+  Movable &operator=(Movable &&) = default;
+  ~Movable();
+
+  void memberUse() {}
+};
+
+struct Copyable {
+  Copyable() = default;
+  Copyable(Copyable const &) = default;
+  Copyable(Copyable &&) = default;
+  Copyable &operator=(Copyable const &) = default;
+  Copyable &operator=(Copyable &&) = default;
+  ~Copyable() = default; 
+
+  void memberUse() {}
+};
+// static_assert(!std::is_trivially_copyable_v<Movable>, "Movable must not be trivially copyable");
+
+void valueReceiver(Movable Mov);
+void constRefReceiver(Movable const &Mov);
+
+void valueTester() {
+  Movable Mov{};
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void testUsageInBranch(bool Splitter) {
+  Movable Mov{};
+
+  valueReceiver(Mov);
+  if(Splitter){
+    Mov.memberUse();
+  } else {
+    Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+
+  if(Splitter){
+    Mov = Movable{};
+  } else {
+    Mov = Movable{};
+  }
+  valueReceiver(Mov);
+  Mov.memberUse();
+}
+
+void testExplicitCopy() {
+  Movable Mov{};
+  constRefReceiver(Movable{Mov});
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: constRefReceiver(Movable{std::move(Mov)});
+}
+
+Movable testReturn() {
+  Movable Mov{};
+  return Mov; // no warning, copy elision
+}
+
+Movable testReturn2(Movable && Mov, bool F) {
+  return F? Mov: Movable{}; 
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: return F? std::move(Mov): Movable{};
+}
+
+void rValReferenceTester(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+  Mov = Movable{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // CHECK-FIXES: valueReceiver(std::move(Mov));
+}
+
+void referenceTester(Movable& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  Mov = Movable{};
+  valueReceiver(Mov);
+}
+
+void pointerTester(Movable* Mov) {
+  valueReceiver(*Mov);
+  valueReceiver(*Mov);
+  *Mov = Movable{};
+  valueReceiver(*Mov);
+}
+
+// Replacements in expansions from macros or of their parameters are buggy, so we don't fix them.
+// Todo (future): The source location of macro parameters might be fixed in the future
+#define FUN(Mov) valueReceiver((Mov))
+void falseMacroExpansion() {
+  Movable Mov;
+  FUN(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] 
+  // CHECK-FIXES: FUN(Mov);
+}
+
+template <class T>
+struct RemoveRef{
+  using type = T;
+};
+
+template <class T>
+struct RemoveRef<T&&>{
+  using type = T;
+};
+
+template <class T>
+struct RemoveRef<T&>{
+  using type = T;
+};
+
+template<class T>
+using RemoveRefT = typename RemoveRef<T>::type;
+
+template <class Movable>
+void initSomething(Movable&& Mov) {
+  valueReceiver(Mov);
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' may be copied on last use, consider forwarding it instead. [performance-unnecessary-copy-on-last-use] 
+  Mov = RemoveRefT<Movable>{};
+  valueReceiver(Mov);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Mov' may be copied on last use, consider forwarding it instead. [performance-unnecessary-copy-on-last-use]
+}
+
+void initSomethingVal(Movable const& Mov) {
+  initSomething<Movable>(Movable{Mov}); 
+}
+
+void initSomethingRValRef(Movable const& Mov) {
+  initSomething<Movable&&>(Movable{Mov}); 
+}
+
+void initSomethingRef(Movable& Mov) {
+  initSomething<Movable&>(Mov); 
+}
+
+// no "automatic - storage" tests:
+
+void staticTester() {
+  static Movable MovStatic;
+  valueReceiver(MovStatic);
+  valueReceiver(MovStatic);
+  MovStatic = Movable{};
+  valueReceiver(MovStatic);
+}
+
+void threadLocalTester() {
+  thread_local Movable MovThreadLocal;
+  valueReceiver(MovThreadLocal);
+  valueReceiver(MovThreadLocal);
+  MovThreadLocal = Movable{};
+  valueReceiver(MovThreadLocal);
+}
+
+void externTester() {
+  extern Movable MovExtern;
+  valueReceiver(MovExtern);
+  valueReceiver(MovExtern);
+  MovExtern = Movable{};
+  valueReceiver(MovExtern);
+}
+
+Movable MovGlobal;
+void globalTester() {
+  valueReceiver(MovGlobal);
+  valueReceiver(MovGlobal);
+  MovGlobal = Movable{};
+  valueReceiver(MovGlobal);
+}
+
+// lambda tests:
+void lambdaCaptureRefTester() {
+  Movable Mov{};
+  auto Lambda = [&Mov](){
+    Mov.memberUse();
+  };
+  Lambda();
+}
+
+void lambdaCaptureValueTester() {
+  Movable Mov{};
+  auto Lambda = [Mov]() mutable {
+    // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+    // CHECK-FIXES: auto Lambda = [Mov]() mutable {
+    // Note: No fix, because a fix requires c++14.
+    Mov.memberUse();
+    };
+  Lambda();
+}
+
+/* Todo (improvement): currently the following is not fixed. 
+        A differentiation between init-params in lambdas is required.
+*/
+void lambdaCaptureValueTester2() {
+  Movable Mov{};
+  auto Lambda = [Mov = Mov]() mutable {
+    // CHECK-MESSAGES: [[@LINE-1]]:24: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+    // NOCHECK-FIXES: auto Lambda = [Mov = std::move(Mov)]() mutable {
+    // Note: No fix, because a fix requires c++14.
+    Mov.memberUse();
+  };
+  Lambda();
+}
+
+void lambdaCaptureValueTester3() {
+  Movable Mov{};
+  auto Lambda = [=]() mutable {
+    // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+    // CHECK-FIXES: auto Lambda = [=]() mutable {
+    // NOCHECK-FIXES: auto Lambda = [=, Mov = std::move(Mov)]() mutable {
+    // Note: No fix, because a fix requires c++14.
+    Mov.memberUse();
+  };
+  Lambda();
+}
+
+/* 
+Todo (improvement):
+Currently the check does not find last usages of fields of local objects.
+
+struct MoveOwner{
+  Movable Mov{};
+};
+
+void testFieldMove(){
+  MoveOwner Owner{};
+  valueReceiver(Owner.Mov);
+  Owner.Mov = Movable{};
+  valueReceiver(Owner.Mov);
+  // DISABLED-CHECK-MESSAGES: [[@LINE-1]]:17: warning: Parameter 'Owner.Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
+  // DISABLED-CHECK-FIXES: valueReceiver(std::move(Owner.Mov));
+}
+*/
+
+/*
+Todo (improvement):
+Currently a heuristic detection of guards is not implemented, so this test is disabled
+But before this is done, the check should not be used for automatic fixing
+
+using NoMove = std::shared_ptr<std::lock_guard<std::mutex>>;
+
+void shareMutex(NoMove Nmov);
+
+void testNoMove(std::mutex& M, int& I) {
+    NoMove Nmov = std::make_shared<std::lock_guard<std::mutex>>(M);
+    shareMutex(Nmov);
+    I = 42;
+}
+*/