New check: use memcpy/memset instead of loop (#4257)

lib/astutils.cpp

@@ -1728,6 +1728,8 @@ static bool functionModifiesArguments(const Function* f)
 
 bool isConstFunctionCall(const Token* ftok, const Library& library)
 {
+    if (isSizeOfEtc(ftok))
+        return true;
     if (!Token::Match(ftok, "%name% ("))
         return false;
     if (const Function* f = ftok->function()) {

lib/checkfunctions.cpp

@@ -689,3 +689,112 @@ void CheckFunctions::copyElisionError(const Token *tok)
                 "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");
 }
+
+void CheckFunctions::useStandardLibrary()
+{
+    if (!mSettings->severity.isEnabled(Severity::style))
+        return;
+
+    for (const Scope& scope: mTokenizer->getSymbolDatabase()->scopeList) {
+        if (scope.type != Scope::ScopeType::eFor)
+            continue;
+
+        const Token *forToken = scope.classDef;
+        // for ( initToken ; condToken ; stepToken )
+        const Token* initToken = getInitTok(forToken);
+        if (!initToken)
+            continue;
+        const Token* condToken = getCondTok(forToken);
+        if (!condToken)
+            continue;
+        const Token* stepToken = getStepTok(forToken);
+        if (!stepToken)
+            continue;
+
+        // 1. we expect that idx variable will be initialized with 0
+        const Token* idxToken = initToken->astOperand1();
+        const Token* initVal = initToken->astOperand2();
+        if (!idxToken || !initVal || !initVal->hasKnownIntValue() || initVal->getKnownIntValue() != 0)
+            continue;
+        const auto idxVarId = idxToken->varId();
+        if (0 == idxVarId)
+            continue;
+
+        // 2. we expect that idx will be less of some variable
+        if (!condToken->isComparisonOp())
+            continue;
+
+        const auto& secondOp = condToken->str();
+        const bool isLess = "<" == secondOp &&
+                            isConstExpression(condToken->astOperand2(), mSettings->library, true, mTokenizer->isCPP()) &&
+                            condToken->astOperand1()->varId() == idxVarId;
+        const bool isMore = ">" == secondOp &&
+                            isConstExpression(condToken->astOperand1(), mSettings->library, true, mTokenizer->isCPP()) &&
+                            condToken->astOperand2()->varId() == idxVarId;
+
+        if (!(isLess || isMore))
+            continue;
+
+        // 3. we expect idx incrementing by 1
+        const bool inc = stepToken->str() == "++" && stepToken->astOperand1()->varId() == idxVarId;
+        const bool plusOne = stepToken->isBinaryOp() && stepToken->str() == "+=" &&
+                             stepToken->astOperand1()->varId() == idxVarId &&
+                             stepToken->astOperand2()->str() == "1";
+        if (!inc && !plusOne)
+            continue;
+
+        // technically using void* here is not correct but some compilers could allow it
+
+        const Token *tok = scope.bodyStart;
+        const std::string memcpyName = mTokenizer->isCPP() ? "std::memcpy" : "memcpy";
+        // (reinterpret_cast<uint8_t*>(dest))[i] = (reinterpret_cast<const uint8_t*>(src))[i];
+        if (Token::Match(tok, "{ (| reinterpret_cast < uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] = "
+                         "(| reinterpret_cast < const| uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] ; }", idxVarId)) {
+            useStandardLibraryError(tok->next(), memcpyName);
+            continue;
+        }
+
+        // ((char*)dst)[i] = ((const char*)src)[i];
+        if (Token::Match(tok, "{ ( ( uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] = "
+                         "( ( const| uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] ; }", idxVarId)) {
+            useStandardLibraryError(tok->next(), memcpyName);
+            continue;
+        }
+
+
+        const static std::string memsetName = mTokenizer->isCPP() ? "std::memset" : "memset";
+        // ((char*)dst)[i] = 0;
+        if (Token::Match(tok, "{ ( ( uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] = %char%|%num% ; }", idxVarId)) {
+            useStandardLibraryError(tok->next(), memsetName);
+            continue;
+        }
+
+        // ((char*)dst)[i] = (const char*)0;
+        if (Token::Match(tok, "{ ( ( uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] = "
+                         "( const| uint8_t|int8_t|char ) (| %char%|%num% )| ; }", idxVarId)) {
+            useStandardLibraryError(tok->next(), memsetName);
+            continue;
+        }
+
+        // (reinterpret_cast<uint8_t*>(dest))[i] = static_cast<const uint8_t>(0);
+        if (Token::Match(tok, "{ (| reinterpret_cast < uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] = "
+                         "(| static_cast < const| uint8_t|int8_t|char > ( %char%|%num% ) )| ; }", idxVarId)) {
+            useStandardLibraryError(tok->next(), memsetName);
+            continue;
+        }
+
+        // (reinterpret_cast<int8_t*>(dest))[i] = 0;
+        if (Token::Match(tok, "{ (| reinterpret_cast < uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] = "
+                         "%char%|%num% ; }", idxVarId)) {
+            useStandardLibraryError(tok->next(), memsetName);
+            continue;
+        }
+    }
+}
+
+void CheckFunctions::useStandardLibraryError(const Token *tok, const std::string& expected)
+{
+    reportError(tok, Severity::style,
+                "useStandardLibrary",
+                "Consider using " + expected + " instead of loop.");
+}

lib/checkfunctions.h

@@ -73,6 +73,7 @@ public:
         checkFunctions.memsetZeroBytes();
         checkFunctions.memsetInvalid2ndParam();
         checkFunctions.returnLocalStdMove();
+        checkFunctions.useStandardLibrary();
     }
 
     /** Check for functions that should not be used */
@@ -103,6 +104,8 @@ public:
     /** @brief %Check for copy elision by RVO|NRVO */
     void returnLocalStdMove();
 
+    void useStandardLibrary();
+
     /** @brief --check-library: warn for unconfigured function calls */
     void checkLibraryMatchFunctions();
 
@@ -122,6 +125,7 @@ private:
     void memsetValueOutOfRangeError(const Token *tok, const std::string &value);
     void missingReturnError(const Token *tok);
     void copyElisionError(const Token *tok);
+    void useStandardLibraryError(const Token *tok, const std::string& expected);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override {
         CheckFunctions c(nullptr, settings, errorLogger);
@@ -141,6 +145,7 @@ private:
         c.memsetValueOutOfRangeError(nullptr,  "varname");
         c.missingReturnError(nullptr);
         c.copyElisionError(nullptr);
+        c.useStandardLibraryError(nullptr, "memcpy");
     }
 
     static std::string myName() {
@@ -156,7 +161,8 @@ private:
                "- 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"
-               "- copy elision optimization for returning value affected by std::move\n";
+               "- copy elision optimization for returning value affected by std::move\n"
+               "- use memcpy()/memset() instead of for loop\n";
     }
 };
 /// @}

lib/token.h

@@ -401,6 +401,9 @@ public:
     bool isEnumerator() const {
         return mTokType == eEnumerator;
     }
+    bool isVariable() const {
+        return mTokType == eVariable;
+    }
     bool isOp() const {
         return (isConstOp() ||
                 isAssignmentOp() ||

test/testfunctions.cpp

@@ -102,6 +102,21 @@ private:
         TEST_CASE(negativeMemoryAllocationSizeError); // #389
 
         TEST_CASE(checkLibraryMatchFunctions);
+
+        TEST_CASE(checkUseStandardLibrary1);
+        TEST_CASE(checkUseStandardLibrary2);
+        TEST_CASE(checkUseStandardLibrary3);
+        TEST_CASE(checkUseStandardLibrary4);
+        TEST_CASE(checkUseStandardLibrary5);
+        TEST_CASE(checkUseStandardLibrary6);
+        TEST_CASE(checkUseStandardLibrary7);
+        TEST_CASE(checkUseStandardLibrary8);
+        TEST_CASE(checkUseStandardLibrary9);
+        TEST_CASE(checkUseStandardLibrary10);
+        TEST_CASE(checkUseStandardLibrary11);
+        TEST_CASE(checkUseStandardLibrary12);
+        TEST_CASE(checkUseStandardLibrary13);
+        TEST_CASE(checkUseStandardLibrary14);
     }
 
 #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
@@ -1853,6 +1868,127 @@ private:
         settings.severity = severity_old;
         settings.checkLibrary = false;
     }
+
+    void checkUseStandardLibrary1() {
+        check("void f(void* dest, void const* src, const size_t count) {\n"
+              "    size_t i;\n"
+              "    for (i = 0; count > i; ++i)\n"
+              "        (reinterpret_cast<uint8_t*>(dest))[i] = (reinterpret_cast<const uint8_t*>(src))[i];\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memcpy instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary2() {
+        check("void f(void* dest, void const* src, const size_t count) {\n"
+              "    for (size_t i = 0; i < count; i++) {\n"
+              "        (reinterpret_cast<uint8_t*>(dest))[i] = (reinterpret_cast<const uint8_t*>(src))[i];\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memcpy instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary3() {
+        check("void f(void* dst, const void* src, const size_t count) {\n"
+              "    size_t i;\n"
+              "    for (i = 0; count > i; i++)\n"
+              "        ((char*)dst)[i] = ((const char*)src)[i];\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memcpy instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary4() {
+        check("void f(void* dst, void* src, const size_t size) {\n"
+              "    for (size_t i = 0; i < size; i += 1) {\n"
+              "        ((int8_t*)dst)[i] = ((int8_t*)src)[i];\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memcpy instead of loop.\n", errout.str());
+    }
+
+    // different indexes
+    void checkUseStandardLibrary5() {
+        check("void f(void* dst, void* src, const size_t size, const size_t from_idx) {\n"
+              "    for (size_t i = 0; i < size; ++i) {\n"
+              "        ((int8_t*)dst)[i] = ((int8_t*)src)[from_idx];\n"
+              "}}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+    // unknown count
+    void checkUseStandardLibrary6() {
+        check("void f(void* dst, void* src, const size_t size) {\n"
+              "    for (size_t i = 0; ((int8_t*)src)[i] != 0; ++i) {\n"
+              "        ((int8_t*)dst)[i] = ((int8_t*)src)[i];\n"
+              "}}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+    // increment with 2
+    void checkUseStandardLibrary7() {
+        check("void f(void* dst, void* src, const size_t size) {\n"
+              "    for (size_t i = 0; i < size; i += 2) {\n"
+              "        ((int8_t*)dst)[i] = ((int8_t*)src)[i];\n"
+              "}}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+    // right argument of assignment could be static_cast, functional cast, c-style and implicit cast
+    // functional cast case not covered
+    void checkUseStandardLibrary8() {
+        check("void f(void* dest, const size_t count) {\n"
+              "    size_t i;\n"
+              "    for (i = 0; i < count; ++i)\n"
+              "        (reinterpret_cast<int8_t*>(dest))[i] = static_cast<const int8_t>(0);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary9() {
+        check("void f(void* dest, const size_t count) {\n"
+              "    for (size_t i = 0; i < count; i++) {\n"
+              "        (reinterpret_cast<uint8_t*>(dest))[i] = (static_cast<const uint8_t>(0));\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary10() {
+        check("void f(void* dst, const size_t size) {\n"
+              "    size_t i;\n"
+              "    for (i = 0; i < size; i++)\n"
+              "        ((char*)dst)[i] = (const char)0;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary11() {
+        check("void f(void* dst, const size_t size) {\n"
+              "    for (size_t i = 0; i < size; i += 1) {\n"
+              "        ((int8_t*)dst)[i] = ((int8_t)0);\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary12() {
+        check("void f(void* dst, const size_t size) {\n"
+              "    for (size_t i = 0; i < size; i += 1) {\n"
+              "        ((int8_t*)dst)[i] = 42;\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary13() {
+        check("void f(void* dest, const size_t count) {\n"
+              "    for (size_t i = 0; i < count; i++) {\n"
+              "        reinterpret_cast<unsigned char*>(dest)[i] = '0';\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
+
+    void checkUseStandardLibrary14() {
+        check("void f(void* dest) {\n"
+              "    for (size_t i = 0; i < sizeof(int)*(15 + 42/2 - 7); i++) {\n"
+              "        reinterpret_cast<unsigned char*>(dest)[i] = '0';\n"
+              "}}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestFunctions)