Improved static string comparision check: Implemented #3214

Fixed false negative on argument count of fnprintf/snprintf when first variable argument is a string. (#3655)
Uncommented call of virtualDestructorError in getErrorMessages in checkclass.h
Refactorizations:
- Rearranged code in checkother.h to make ordering more consistent and to increase encapsulation of private data
- Replaced some single-token-patterns

lib/checkclass.h

@@ -131,7 +131,7 @@ private:
         c.unusedPrivateFunctionError(0, "classname", "funcname");
         c.memsetError(0, "memfunc", "classname", "class");
         c.operatorEqReturnError(0, "class");
-        //c.virtualDestructorError(0, "Base", "Derived");
+        c.virtualDestructorError(0, "Base", "Derived");
         c.thisSubtractionError(0);
         c.operatorEqRetRefThisError(0);
         c.operatorEqToSelfError(0);
@@ -175,10 +175,10 @@ private:
         Usage() : assign(false), init(false) { }
 
         /** @brief has this variable been assigned? */
-        bool        assign;
+        bool assign;
 
         /** @brief has this variable been initialized? */
-        bool        init;
+        bool init;
     };
 
     bool isBaseClassFunc(const Token *tok, const Scope *scope);

lib/checkother.cpp

@@ -477,7 +477,7 @@ void CheckOther::checkSizeofForArrayParameter()
                         while (declTok->str() == "[") {
                             declTok = declTok->link()->next();
                         }
-                        if (!(Token::Match(declTok, "= %str%")) && !(Token::simpleMatch(declTok, "= {")) && !(Token::simpleMatch(declTok, ";"))) {
+                        if (!(Token::Match(declTok, "= %str%")) && !(Token::simpleMatch(declTok, "= {")) && declTok->str() != ";") {
                             if (declTok->str() == ",") {
                                 while (declTok->str() != ";") {
                                     if (declTok->str() == ")") {
@@ -1344,8 +1344,7 @@ void CheckOther::checkWrongPrintfScanfArguments()
             if (Token::Match(formatStringTok, "%str% ,")) {
                 argListTok = formatStringTok->nextArgument(); // Find fourth parameter (first argument of va_args)
                 formatString = formatStringTok->str();
-            }
+            } else if (Token::Match(formatStringTok, "%str% )")) {
-            if (Token::Match(formatStringTok, "%str% )")) {
                 argListTok = 0; // Find fourth parameter (first argument of va_args)
                 formatString = formatStringTok->str();
             } else {
@@ -1401,7 +1400,7 @@ void CheckOther::checkWrongPrintfScanfArguments()
                     numFormat++;
 
                     // Perform type checks
-                    if (_settings->isEnabled("style") && Token::Match(argListTok, "%any% ,|)")) { // We can currently only check the type of arguments matching this simple pattern.
+                    if (_settings->isEnabled("style") && argListTok && Token::Match(argListTok->next(), "[,)]")) { // We can currently only check the type of arguments matching this simple pattern.
                         const Variable* variableInfo = symbolDatabase->getVariableFromVarId(argListTok->varId());
                         const Token* varTypeTok = variableInfo ? variableInfo->typeStartToken() : NULL;
                         if (varTypeTok && varTypeTok->str() == "static")
@@ -1906,7 +1905,7 @@ void CheckOther::lookupVar(const Token *tok1, const std::string &varname)
             if (indentlevel == 0)
                 return;
             used1 = true;
-            if (for_or_while && !Token::simpleMatch(tok->next(), "="))
+            if (for_or_while && tok->strAt(1) != "=")
                 used2 = true;
             if (used1 && used2)
                 return;
@@ -1916,7 +1915,7 @@ void CheckOther::lookupVar(const Token *tok1, const std::string &varname)
             // %unknown% ( %any% ) {
             // If %unknown% is anything except if, we assume
             // that it is a for or while loop or a macro hiding either one
-            if (Token::simpleMatch(tok->next(), "(") &&
+            if (tok->strAt(1) == "(" &&
                 Token::simpleMatch(tok->next()->link(), ") {")) {
                 if (tok->str() != "if")
                     for_or_while = true;
@@ -2971,35 +2970,45 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare()
 
     const Token *tok = _tokenizer->tokens();
     while (tok && (tok = Token::findmatch(tok, pattern1)) != NULL) {
-        alwaysTrueFalseStringCompareError(tok, tok->strAt(2), tok->strAt(4));
+        const std::string &str1 = tok->strAt(2);
+        const std::string &str2 = tok->strAt(4);
+        alwaysTrueFalseStringCompareError(tok, str1, str2, str1==str2);
         tok = tok->tokAt(5);
     }
 
     tok = _tokenizer->tokens();
     while (tok && (tok = Token::findmatch(tok, pattern2)) != NULL) {
-        alwaysTrueFalseStringCompareError(tok, tok->strAt(4), tok->strAt(6));
+        const std::string &str1 = tok->strAt(4);
+        const std::string &str2 = tok->strAt(6);
+        alwaysTrueFalseStringCompareError(tok, str1, str2, str1==str2);
         tok = tok->tokAt(7);
     }
 
     tok = _tokenizer->tokens();
     while (tok && (tok = Token::findmatch(tok, pattern3)) != NULL) {
-        const Token *var1 = tok->tokAt(2);
+        const std::string &str1 = tok->strAt(2);
-        const Token *var2 = tok->tokAt(4);
+        const std::string &str2 = tok->strAt(4);
-        const std::string &str1 = var1->str();
-        const std::string &str2 = var2->str();
         if (str1 == str2)
             alwaysTrueStringVariableCompareError(tok, str1, str2);
         tok = tok->tokAt(5);
     }
+
+    tok = _tokenizer->tokens();
+    while (tok && (tok = Token::findmatch(tok, "!!+ %str% ==|!= %str% !!+")) != NULL) {
+        const std::string &str1 = tok->strAt(1);
+        const std::string &str2 = tok->strAt(3);
+        alwaysTrueFalseStringCompareError(tok, str1, str2, str1==str2);
+        tok = tok->tokAt(5);
+    }
 }
 
-void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2)
+void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2, bool warning)
 {
     const std::size_t stringLen = 10;
     const std::string string1 = (str1.size() < stringLen) ? str1 : (str1.substr(0, stringLen-2) + "..");
     const std::string string2 = (str2.size() < stringLen) ? str2 : (str2.substr(0, stringLen-2) + "..");
 
-    if (str1 == str2) {
+    if (warning) {
         reportError(tok, Severity::warning, "staticStringCompare",
                     "Comparison of always identical static strings.\n"
                     "The compared strings, '" + string1 + "' and '" + string2 + "', are always identical. "

lib/checkother.h

@@ -112,11 +112,9 @@ public:
 
     /** @brief Clarify calculation for ".. a * b ? .." */
     void clarifyCalculation();
-    void clarifyCalculationError(const Token *tok, const std::string &op);
 
     /** @brief Suspicious condition (assignment+comparison) */
     void clarifyCondition();
-    void clarifyConditionError(const Token *tok, bool assign, bool boolop);
 
     /** @brief Are there C-style pointer casts in a c++ file? */
     void warningOldStylePointerCast();
@@ -167,28 +165,15 @@ public:
 
     /** @brief %Check for 'sizeof sizeof ..' */
     void sizeofsizeof();
-    void sizeofsizeofError(const Token *tok);
 
     /** @brief %Check for calculations inside sizeof */
     void sizeofCalculation();
-    void sizeofCalculationError(const Token *tok);
 
     /** @brief scanf can crash if width specifiers are not used */
     void invalidScanf();
-    void invalidScanfError(const Token *tok);
 
     /** @brief %Checks type and number of arguments given to functions like printf or scanf*/
     void checkWrongPrintfScanfArguments();
-    void wrongPrintfScanfArgumentsError(const Token* tok,
-                                        const std::string &function,
-                                        unsigned int numFormat,
-                                        unsigned int numFunction);
-    void invalidScanfArgTypeError(const Token* tok, const std::string &functionName, unsigned int numFormat);
-    void invalidPrintfArgTypeError_s(const Token* tok, unsigned int numFormat);
-    void invalidPrintfArgTypeError_n(const Token* tok, unsigned int numFormat);
-    void invalidPrintfArgTypeError_p(const Token* tok, unsigned int numFormat);
-    void invalidPrintfArgTypeError_int(const Token* tok, unsigned int numFormat, char c);
-    void invalidPrintfArgTypeError_float(const Token* tok, unsigned int numFormat, char c);
 
     /** @brief %Check for assigning to the same variable twice in a switch statement*/
     void checkRedundantAssignmentInSwitch();
@@ -265,7 +250,23 @@ public:
     /** @brief %Check for double free or double close operations */
     void checkDoubleFree();
 
+private:
     // Error messages..
+    void clarifyCalculationError(const Token *tok, const std::string &op);
+    void clarifyConditionError(const Token *tok, bool assign, bool boolop);
+    void sizeofsizeofError(const Token *tok);
+    void sizeofCalculationError(const Token *tok);
+    void invalidScanfError(const Token *tok);
+    void wrongPrintfScanfArgumentsError(const Token* tok,
+                                        const std::string &function,
+                                        unsigned int numFormat,
+                                        unsigned int numFunction);
+    void invalidScanfArgTypeError(const Token* tok, const std::string &functionName, unsigned int numFormat);
+    void invalidPrintfArgTypeError_s(const Token* tok, unsigned int numFormat);
+    void invalidPrintfArgTypeError_n(const Token* tok, unsigned int numFormat);
+    void invalidPrintfArgTypeError_p(const Token* tok, unsigned int numFormat);
+    void invalidPrintfArgTypeError_int(const Token* tok, unsigned int numFormat, char c);
+    void invalidPrintfArgTypeError_float(const Token* tok, unsigned int numFormat, char c);
     void cstyleCastError(const Token *tok);
     void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to);
     void dangerousUsageStrtolError(const Token *tok);
@@ -301,7 +302,7 @@ public:
     void duplicateIfError(const Token *tok1, const Token *tok2);
     void duplicateBranchError(const Token *tok1, const Token *tok2);
     void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op);
-    void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2);
+    void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2, bool warning);
     void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2);
     void duplicateBreakError(const Token *tok);
     void unreachableCodeError(const Token* tok);
@@ -360,7 +361,7 @@ public:
         c.duplicateIfError(0, 0);
         c.duplicateBranchError(0, 0);
         c.duplicateExpressionError(0, 0, "&&");
-        c.alwaysTrueFalseStringCompareError(0, "str1", "str2");
+        c.alwaysTrueFalseStringCompareError(0, "str1", "str2", true);
         c.alwaysTrueStringVariableCompareError(0, "varname1", "varname2");
         c.duplicateBreakError(0);
         c.unreachableCodeError(0);
@@ -438,8 +439,6 @@ public:
                "* optimisation: detect post increment/decrement\n";
     }
 
-private:
-
     /**
      * @brief Used in warningRedundantCode()
      * Iterates through the %var% tokens in a fully qualified name and concatenates them.
@@ -452,7 +451,7 @@ private:
             *tok = (*tok)->tokAt(2);
         }
 
-        if (Token::Match(*tok, "%var%"))
+        if ((*tok)->isName())
             varname.append((*tok)->str());
 
         return varname;

test/testother.cpp

@@ -2207,7 +2207,7 @@ private:
               "    fprintf(stderr,\"%u%s\");\n"
               "    snprintf(str,10,\"%u%s\");\n"
               "    sprintf(string1, \"%-*.*s\", 32, string2);\n" // #3364
-              "    sprintf(string1, \"%*\", 32);\n" // #3364
+              "    snprintf(a, 9, \"%s%d\", \"11223344\");\n" // #3655
               "}");
         ASSERT_EQUALS("[test.cpp:2]: (error) printf format string has 1 parameters but only 0 are given\n"
                       "[test.cpp:3]: (error) printf format string has 2 parameters but only 1 are given\n"
@@ -2216,7 +2216,8 @@ private:
                       "[test.cpp:6]: (error) printf format string has 3 parameters but only 2 are given\n"
                       "[test.cpp:7]: (error) fprintf format string has 2 parameters but only 0 are given\n"
                       "[test.cpp:8]: (error) snprintf format string has 2 parameters but only 0 are given\n"
-                      "[test.cpp:9]: (error) sprintf format string has 3 parameters but only 2 are given\n", errout.str());
+                      "[test.cpp:9]: (error) sprintf format string has 3 parameters but only 2 are given\n"
+                      "[test.cpp:10]: (error) snprintf format string has 2 parameters but only 1 are given\n", errout.str());
 
         check("void foo(char *str) {\n"
               "    printf(\"\", 0);\n"
@@ -2239,6 +2240,7 @@ private:
               "    fprintf(stderr, \"error: %m\n\");\n" // #3339
               "    printf(\"string: %.*s\n\", len, string);\n" // #3311
               "    fprintf(stderr, \"%*cText.\n\", indent, ' ');\n" // #3313
+              "    sprintf(string1, \"%*\", 32);\n" // #3364
               "}");
         ASSERT_EQUALS("", errout.str());
 
@@ -4138,6 +4140,30 @@ private:
             "  }"
             "}");
         ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str());
+
+        check_preprocess_suppress(
+            "int main() {\n"
+            "  if (\"str\" == \"str\") {\n"
+            "    std::cout << \"Equal\n\"\n"
+            "  }\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of always identical static strings.\n", errout.str());
+
+        check_preprocess_suppress(
+            "int main() {\n"
+            "  if (\"str\" != \"str\") {\n"
+            "    std::cout << \"Equal\n\"\n"
+            "  }\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of always identical static strings.\n", errout.str());
+
+        check_preprocess_suppress(
+            "int main() {\n"
+            "  if (a+\"str\" != \"str\"+b) {\n"
+            "    std::cout << \"Equal\n\"\n"
+            "  }\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void checkStrncmpSizeof() {