[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

[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