New check: [perf] Copy elision optimization can't be applied for `return std::move(local)` (#3281)

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");
+}

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";
     }
 };
 /// @}

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