From 6b8d0be431e820bd44dc25e0a65849daae365d28 Mon Sep 17 00:00:00 2001 From: Maksim Derbasov Date: Tue, 6 Jul 2021 09:07:46 +0300 Subject: [PATCH] New check: [perf] Copy elision optimization can't be applied for `return std::move(local)` (#3281) --- lib/checkfunctions.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ lib/checkfunctions.h | 9 ++++++++- test/testfunctions.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 1 deletion(-) diff --git a/lib/checkfunctions.cpp b/lib/checkfunctions.cpp index c6744b591..9a53f24b3 100644 --- a/lib/checkfunctions.cpp +++ b/lib/checkfunctions.cpp @@ -541,3 +541,43 @@ void CheckFunctions::checkLibraryMatchFunctions() "--check-library: There is no matching configuration for function " + functionName + "()"); } } + +// Check for problems to compiler apply (Named) Return Value Optimization for local variable +// Technically we have different guarantees between standard versions +// details: https://en.cppreference.com/w/cpp/language/copy_elision +void CheckFunctions::returnLocalStdMove() +{ + if(!mTokenizer->isCPP() || mSettings->standards.cpp < Standards::CPP11) + return; + + if (!mSettings->severity.isEnabled(Severity::performance)) + return; + + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope *scope : symbolDatabase->functionScopes) { + // Expect return by-value + if (Function::returnsReference(scope->function, true)) + continue; + const auto rets = Function::findReturns(scope->function); + for (const Token* ret : rets) { + if (!Token::simpleMatch(ret->tokAt(-3), "std :: move (")) + continue; + const Token* retval = ret->astOperand2(); + // NRVO + if (retval->variable() && retval->variable()->isLocal() && !retval->variable()->isVolatile()) + copyElisionError(retval); + // RVO + if (Token::Match(retval, "(|{") && !retval->isCast()) + copyElisionError(retval); + } + } +} + +void CheckFunctions::copyElisionError(const Token *tok) +{ + reportError(tok, + Severity::performance, + "returnStdMoveLocal", + "Using std::move for returning object by-value from function will affect copy elision optimization." + " More: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-move-local"); +} diff --git a/lib/checkfunctions.h b/lib/checkfunctions.h index fbba09696..30dd816bd 100644 --- a/lib/checkfunctions.h +++ b/lib/checkfunctions.h @@ -74,6 +74,7 @@ public: checkFunctions.checkMathFunctions(); checkFunctions.memsetZeroBytes(); checkFunctions.memsetInvalid2ndParam(); + checkFunctions.returnLocalStdMove(); } /** Check for functions that should not be used */ @@ -101,6 +102,9 @@ public: /** @brief %Check for invalid 2nd parameter of memset() */ void memsetInvalid2ndParam(); + /** @brief %Check for copy elision by RVO|NRVO */ + void returnLocalStdMove(); + /** @brief --check-library: warn for unconfigured function calls */ void checkLibraryMatchFunctions(); @@ -119,6 +123,7 @@ private: void memsetFloatError(const Token *tok, const std::string &var_value); void memsetValueOutOfRangeError(const Token *tok, const std::string &value); void missingReturnError(const Token *tok); + void copyElisionError(const Token *tok); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { CheckFunctions c(nullptr, settings, errorLogger); @@ -137,6 +142,7 @@ private: c.memsetFloatError(nullptr, "varname"); c.memsetValueOutOfRangeError(nullptr, "varname"); c.missingReturnError(nullptr); + c.copyElisionError(nullptr); } static std::string myName() { @@ -151,7 +157,8 @@ private: "- Warn if a function is called whose usage is discouraged\n" "- memset() third argument is zero\n" "- memset() with a value out of range as the 2nd parameter\n" - "- memset() with a float as the 2nd parameter\n"; + "- memset() with a float as the 2nd parameter\n" + "- copy elision optimization for returning value affected by std::move\n"; } }; /// @} diff --git a/test/testfunctions.cpp b/test/testfunctions.cpp index 96b954e6c..f728ba8c4 100644 --- a/test/testfunctions.cpp +++ b/test/testfunctions.cpp @@ -37,6 +37,7 @@ private: void run() OVERRIDE { settings.severity.enable(Severity::style); settings.severity.enable(Severity::warning); + settings.severity.enable(Severity::performance); settings.severity.enable(Severity::portability); settings.libraries.emplace_back("posix"); settings.standards.c = Standards::C11; @@ -84,6 +85,14 @@ private: // missing "return" TEST_CASE(checkMissingReturn); + + // std::move for locar variable + TEST_CASE(returnLocalStdMove1); + TEST_CASE(returnLocalStdMove2); + TEST_CASE(returnLocalStdMove3); + TEST_CASE(returnLocalStdMove4); + + TEST_CASE(returnLocalStdMove5); } void check(const char code[], const char filename[]="test.cpp", const Settings* settings_=nullptr) { @@ -1405,6 +1414,38 @@ private: " else\n" " return {1, 2};\n" "}"); + } + // NRVO check + void returnLocalStdMove1() { + check("struct A{}; A f() { A var; return std::move(var); }"); + ASSERT_EQUALS("[test.cpp:1]: (performance) Using std::move for returning object by-value from function will affect copy elision optimization." + " More: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-move-local\n", errout.str()); + } + + // RVO, C++03 ctor style + void returnLocalStdMove2() { + check("struct A{}; A f() { return std::move( A() ); }"); + ASSERT_EQUALS("[test.cpp:1]: (performance) Using std::move for returning object by-value from function will affect copy elision optimization." + " More: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-move-local\n", errout.str()); + } + + // RVO, new ctor style + void returnLocalStdMove3() { + check("struct A{}; A f() { return std::move(A{}); }"); + ASSERT_EQUALS("[test.cpp:1]: (performance) Using std::move for returning object by-value from function will affect copy elision optimization." + " More: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-move-local\n", errout.str()); + } + + // Function argument + void returnLocalStdMove4() { + check("struct A{}; A f(A a) { return std::move(A{}); }"); + ASSERT_EQUALS("[test.cpp:1]: (performance) Using std::move for returning object by-value from function will affect copy elision optimization." + " More: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-move-local\n", errout.str()); + } + + void returnLocalStdMove5() { + check("struct A{} a; A f1() { return std::move(a); }\n" + "A f2() { volatile A var; return std::move(var); }"); ASSERT_EQUALS("", errout.str()); } };