Merge branch 'chr_localtype' of https://github.com/chrchr-github/cppc…

…heck into chr_localtype
chrchr-github · Aug 20, 2023 · 035a63f · 035a63f
2 parents 61a8d5a + f13c0e7
commit 035a63f
Show file tree

Hide file tree

Showing 18 changed files with 220 additions and 77 deletions.
diff --git a/lib/astutils.cpp b/lib/astutils.cpp
@@ -1426,7 +1426,7 @@ static bool isForLoopIncrement(const Token* const tok)
            parent->astParent()->astParent()->astOperand1()->str() == "for";
 }
 
-bool isUsedAsBool(const Token* const tok)
+bool isUsedAsBool(const Token* const tok, const Settings* settings)
 {
     if (!tok)
         return false;
@@ -1439,6 +1439,15 @@ bool isUsedAsBool(const Token* const tok)
     const Token* parent = tok->astParent();
     if (!parent)
         return false;
+    if (Token::simpleMatch(parent, "["))
+        return false;
+    if (parent->isUnaryOp("*"))
+        return false;
+    if (Token::simpleMatch(parent, ".")) {
+        if (astIsRHS(tok))
+            return isUsedAsBool(parent, settings);
+        return false;
+    }
     if (Token::Match(parent, "&&|!|%oror%"))
         return true;
     if (parent->isCast())
@@ -1455,7 +1464,7 @@ bool isUsedAsBool(const Token* const tok)
     if (isForLoopCondition(tok))
         return true;
     if (!Token::Match(parent, "%cop%")) {
-        std::vector<ValueType> vtParents = getParentValueTypes(tok);
+        std::vector<ValueType> vtParents = getParentValueTypes(tok, settings);
         return std::any_of(vtParents.cbegin(), vtParents.cend(), [&](const ValueType& vt) {
             return vt.pointer == 0 && vt.type == ValueType::BOOL;
         });

diff --git a/lib/astutils.h b/lib/astutils.h
@@ -254,7 +254,7 @@ const Token* isInLoopCondition(const Token* tok);
 /**
  * Is token used as boolean, that is to say cast to a bool, or used as a condition in a if/while/for
  */
-CPPCHECKLIB bool isUsedAsBool(const Token * const tok);
+CPPCHECKLIB bool isUsedAsBool(const Token* const tok, const Settings* settings = nullptr);
 
 /**
  * Are two conditions opposite

diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp
@@ -60,7 +60,7 @@ bool CheckCondition::diag(const Token* tok, bool insert)
         return false;
     const Token* parent = tok->astParent();
     bool hasParent = false;
-    while (Token::Match(parent, "&&|%oror%")) {
+    while (Token::Match(parent, "!|&&|%oror%")) {
         if (mCondDiags.count(parent) != 0) {
             hasParent = true;
             break;
@@ -410,6 +410,8 @@ void CheckCondition::comparison()
 
 void CheckCondition::comparisonError(const Token *tok, const std::string &bitop, MathLib::bigint value1, const std::string &op, MathLib::bigint value2, bool result)
 {
+    if (tok && (diag(tok) | diag(tok->astParent())))
+        return;
     std::ostringstream expression;
     expression << std::hex << "(X " << bitop << " 0x" << value1 << ") " << op << " 0x" << value2;
 
@@ -1370,6 +1372,8 @@ void CheckCondition::checkModuloAlwaysTrueFalse()
 
 void CheckCondition::moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal)
 {
+    if (diag(tok))
+        return;
     reportError(tok, Severity::warning, "moduloAlwaysTrueFalse",
                 "Comparison of modulo result is predetermined, because it is always less than " + maxVal + ".", CWE398, Certainty::normal);
 }
@@ -1466,7 +1470,7 @@ void CheckCondition::alwaysTrueFalse()
         for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
             if (Token::simpleMatch(tok, "<") && tok->link()) // don't write false positives when templates are used
                 continue;
-            if (!tok->hasKnownBoolValue())
+            if (!tok->hasKnownIntValue())
                 continue;
             if (Token::Match(tok->previous(), "%name% (") && tok->previous()->function()) {
                 const Function* f = tok->previous()->function();
@@ -1490,7 +1494,7 @@ void CheckCondition::alwaysTrueFalse()
                 else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() &&
                          Token::simpleMatch(parent->astParent()->astParent()->previous(), "for ("))
                     condition = parent->astParent()->astParent()->previous();
-                else if (isBooleanFuncArg(tok))
+                else if (Token::Match(tok, "%comp%"))
                     condition = tok;
                 else
                     continue;
@@ -1582,10 +1586,7 @@ void CheckCondition::alwaysTrueFalse()
             if (hasSizeof)
                 continue;
 
-            auto it = std::find_if(tok->values().begin(), tok->values().end(), [](const ValueFlow::Value& v) {
-                return v.isIntValue();
-            });
-            alwaysTrueFalseError(tok, condition, &*it);
+            alwaysTrueFalseError(tok, condition, &tok->values().front());
         }
     }
 }

diff --git a/lib/checkcondition.h b/lib/checkcondition.h
@@ -67,12 +67,12 @@ class CPPCHECKLIB CheckCondition : public Check {
         checkCondition.checkPointerAdditionResultNotNull();
         checkCondition.checkDuplicateConditionalAssign();
         checkCondition.assignIf();
-        checkCondition.alwaysTrueFalse();
         checkCondition.checkBadBitmaskCheck();
         checkCondition.comparison();
         checkCondition.checkModuloAlwaysTrueFalse();
         checkCondition.checkAssignmentInCondition();
         checkCondition.checkCompareValueOutOfTypeRange();
+        checkCondition.alwaysTrueFalse();
     }
 
     /** mismatching assignment / comparison */

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
@@ -3640,6 +3640,21 @@ static bool isVariableExpression(const Token* tok)
     return false;
 }
 
+static bool isVariableExprHidden(const Token* tok)
+{
+    if (!tok)
+        return false;
+    if (!tok->astParent())
+        return false;
+    if (Token::simpleMatch(tok->astParent(), "*") && Token::simpleMatch(tok->astSibling(), "0"))
+        return true;
+    if (Token::simpleMatch(tok->astParent(), "&&") && Token::simpleMatch(tok->astSibling(), "false"))
+        return true;
+    if (Token::simpleMatch(tok->astParent(), "||") && Token::simpleMatch(tok->astSibling(), "true"))
+        return true;
+    return false;
+}
+
 void CheckOther::checkKnownArgument()
 {
     if (!mSettings->severity.isEnabled(Severity::style))
@@ -3679,44 +3694,25 @@ void CheckOther::checkKnownArgument()
                     mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true))
                 continue;
             // ensure that there is a integer variable in expression with unknown value
-            std::string varexpr;
-            bool isVariableExprHidden = false;  // Is variable expression explicitly hidden
-            auto setVarExpr = [&varexpr, &isVariableExprHidden](const Token *child) {
+            const Token* vartok = findAstNode(tok, [](const Token* child) {
                 if (Token::Match(child, "%var%|.|[")) {
-                    if (child->valueType() && child->valueType()->pointer == 0 && child->valueType()->isIntegral() && child->values().empty()) {
-                        varexpr = child->expressionString();
-                        return ChildrenToVisit::done;
-                    }
-                    return ChildrenToVisit::none;
+                    return astIsIntegral(child, false) && !astIsPointer(child) && child->values().empty();
                 }
-                if (Token::simpleMatch(child->previous(), "sizeof ("))
-                    return ChildrenToVisit::none;
-
-                // hide variable explicitly with 'x * 0' etc
-                if (!isVariableExprHidden) {
-                    if (Token::simpleMatch(child, "*") && (Token::simpleMatch(child->astOperand1(), "0") || Token::simpleMatch(child->astOperand2(), "0")))
-                        return ChildrenToVisit::none;
-                    if (Token::simpleMatch(child, "&&") && (Token::simpleMatch(child->astOperand1(), "false") || Token::simpleMatch(child->astOperand2(), "false")))
-                        return ChildrenToVisit::none;
-                    if (Token::simpleMatch(child, "||") && (Token::simpleMatch(child->astOperand1(), "true") || Token::simpleMatch(child->astOperand2(), "true")))
-                        return ChildrenToVisit::none;
-                }
-
-                return ChildrenToVisit::op1_and_op2;
-            };
-            visitAstNodes(tok, setVarExpr);
-            if (varexpr.empty()) {
-                isVariableExprHidden = true;
-                visitAstNodes(tok, setVarExpr);
-            }
-            if (varexpr.empty())
+                return false;
+            });
+            if (!vartok)
+                continue;
+            if (vartok->astSibling() &&
+                findAstNode(vartok->astSibling(), [](const Token* child) {
+                return Token::simpleMatch(child, "sizeof");
+            }))
                 continue;
             // ensure that function name does not contain "assert"
-            std::string funcname = tok->astParent()->previous()->str();
+            std::string funcname = ftok->str();
             strTolower(funcname);
             if (funcname.find("assert") != std::string::npos)
                 continue;
-            knownArgumentError(tok, ftok, &tok->values().front(), varexpr, isVariableExprHidden);
+            knownArgumentError(tok, ftok, &tok->values().front(), vartok->expressionString(), isVariableExprHidden(vartok));
         }
     }
 }
@@ -3747,6 +3743,48 @@ void CheckOther::knownArgumentError(const Token *tok, const Token *ftok, const V
     reportError(errorPath, Severity::style, id, errmsg, CWE570, Certainty::normal);
 }
 
+void CheckOther::checkKnownPointerToBool()
+{
+    if (!mSettings->severity.isEnabled(Severity::style))
+        return;
+    const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase();
+    for (const Scope* functionScope : symbolDatabase->functionScopes) {
+        for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) {
+            if (!tok->hasKnownIntValue())
+                continue;
+            if (!astIsPointer(tok))
+                continue;
+            if (Token::Match(tok->astParent(), "?|!|&&|%oror%|%comp%"))
+                continue;
+            if (tok->astParent() && Token::Match(tok->astParent()->previous(), "if|while|switch|sizeof ("))
+                continue;
+            if (tok->isExpandedMacro())
+                continue;
+            if (findParent(tok, [](const Token* parent) {
+                return parent->isExpandedMacro();
+            }))
+                continue;
+            if (!isUsedAsBool(tok, mSettings))
+                continue;
+            const ValueFlow::Value& value = tok->values().front();
+            knownPointerToBoolError(tok, &value);
+        }
+    }
+}
+
+void CheckOther::knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value)
+{
+    if (!tok) {
+        reportError(tok, Severity::style, "knownPointerToBool", "Pointer expression 'p' converted to bool is always true.");
+        return;
+    }
+    std::string cond = value->intvalue ? "true" : "false";
+    const std::string& expr = tok->expressionString();
+    std::string errmsg = "Pointer expression '" + expr + "' converted to bool is always " + cond + ".";
+    const ErrorPath errorPath = getErrorPath(tok, value, errmsg);
+    reportError(errorPath, Severity::style, "knownPointerToBool", errmsg, CWE570, Certainty::normal);
+}
+
 void CheckOther::checkComparePointers()
 {
     const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();

diff --git a/lib/checkother.h b/lib/checkother.h
@@ -85,6 +85,7 @@ class CPPCHECKLIB CheckOther : public Check {
         checkOther.checkFuncArgNamesDifferent();
         checkOther.checkShadowVariables();
         checkOther.checkKnownArgument();
+        checkOther.checkKnownPointerToBool();
         checkOther.checkComparePointers();
         checkOther.checkIncompleteStatement();
         checkOther.checkRedundantCopy();
@@ -218,6 +219,8 @@ class CPPCHECKLIB CheckOther : public Check {
 
     void checkKnownArgument();
 
+    void checkKnownPointerToBool();
+
     void checkComparePointers();
 
     void checkModuloOfOne();
@@ -279,6 +282,7 @@ class CPPCHECKLIB CheckOther : public Check {
     void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector<const Token*> & declarations, const std::vector<const Token*> & definitions);
     void shadowError(const Token *var, const Token *shadowed, std::string type);
     void knownArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value, const std::string &varexpr, bool isVariableExpressionHidden);
+    void knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value);
     void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2);
     void checkModuloOfOneError(const Token *tok);
 
@@ -348,6 +352,7 @@ class CPPCHECKLIB CheckOther : public Check {
         c.shadowError(nullptr, nullptr, "function");
         c.shadowError(nullptr, nullptr, "argument");
         c.knownArgumentError(nullptr, nullptr, nullptr, "x", false);
+        c.knownPointerToBoolError(nullptr, nullptr);
         c.comparePointersError(nullptr, nullptr, nullptr);
         c.redundantAssignmentError(nullptr, nullptr, "var", false);
         c.redundantInitializationError(nullptr, nullptr, "var", false);

diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp
@@ -704,7 +704,7 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const
             else if (i->isArray() && i->nameToken()->previous()->str() == "&")
                 type = Variables::referenceArray;
             else if (i->isArray())
-                type = (i->dimensions().size() == 1U) ? Variables::array : Variables::pointerArray;
+                type = Variables::array;
             else if (i->isReference() && !(i->valueType() && i->valueType()->type == ValueType::UNKNOWN_TYPE && Token::simpleMatch(i->typeStartToken(), "auto")))
                 type = Variables::reference;
             else if (i->nameToken()->previous()->str() == "*" && i->nameToken()->strAt(-2) == "*")
@@ -1069,13 +1069,15 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const
         } else if (Token::Match(tok, "[(,] & %var% [,)]")) {
             variables.eraseAll(tok->tokAt(2)->varId());
         } else if (Token::Match(tok, "[(,] (") &&
-                   Token::Match(tok->next()->link(), ") %var% [,)]")) {
+                   Token::Match(tok->next()->link(), ") %var% [,)[]")) {
             variables.use(tok->next()->link()->next()->varId(), tok);   // use = read + write
-        } else if (Token::Match(tok, "[(,] *| %var% =")) {
-            tok = tok->next();
-            if (tok->str() == "*")
-                tok = tok->next();
-            variables.use(tok->varId(), tok);
+        } else if (Token::Match(tok, "[(,] *| *| %var%")) {
+            const Token* vartok = tok->next();
+            while (vartok->str() == "*")
+                vartok = vartok->next();
+            if (!(vartok->variable() && vartok == vartok->variable()->nameToken()) &&
+                !(tok->str() == "(" && !Token::Match(tok->previous(), "%name%")))
+                variables.use(vartok->varId(), vartok);
         }
 
         // function

diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp
@@ -488,7 +488,7 @@ struct ForwardTraversal {
             if (allAnalysis.isIncremental())
                 return Break(Analyzer::Terminate::Bail);
         } else if (allAnalysis.isModified()) {
-            std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, /*isModified*/ true);
+            std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
             bool forkContinue = true;
             for (ForwardTraversal& ft : ftv) {
                 if (condTok)

diff --git a/lib/token.cpp b/lib/token.cpp
@@ -2388,15 +2388,6 @@ bool Token::hasKnownIntValue() const
     });
 }
 
-bool Token::hasKnownBoolValue() const
-{
-    if (!mImpl->mValues)
-        return false;
-    return std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [](const ValueFlow::Value& value) {
-        return value.isIntValue() && (value.isKnown() || (value.intvalue == 0 && value.isImpossible()));
-    });
-}
-
 bool Token::hasKnownValue() const
 {
     return mImpl->mValues && std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));

diff --git a/lib/token.h b/lib/token.h
@@ -1216,7 +1216,6 @@ class CPPCHECKLIB Token {
     }
 
     bool hasKnownIntValue() const;
-    bool hasKnownBoolValue() const;
     bool hasKnownValue() const;
     bool hasKnownValue(ValueFlow::Value::ValueType t) const;
     bool hasKnownSymbolicValue(const Token* tok) const;

diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -6843,7 +6843,8 @@ static void valueFlowInferCondition(TokenList& tokenlist,
                 }
             }
         } else if (Token::Match(tok->astParent(), "?|&&|!|%oror%") ||
-                   Token::Match(tok->astParent()->previous(), "if|while (")) {
+                   Token::Match(tok->astParent()->previous(), "if|while (") ||
+                   (astIsPointer(tok) && isUsedAsBool(tok, settings))) {
             std::vector<ValueFlow::Value> result = infer(IntegralInferModel{}, "!=", tok->values(), 0);
             if (result.size() != 1)
                 continue;
@@ -8707,10 +8708,12 @@ static MathLib::bigint valueFlowGetStrLength(const Token* tok)
         return Token::getStrLength(tok);
     if (astIsGenericChar(tok) || tok->tokType() == Token::eChar)
         return 1;
-    if (const ValueFlow::Value* v2 = tok->getKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE))
-        return v2->intvalue;
-    if (const ValueFlow::Value* v1 = tok->getKnownValue(ValueFlow::Value::ValueType::TOK))
-        return valueFlowGetStrLength(v1->tokvalue);
+    if (const ValueFlow::Value* v = tok->getKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE))
+        return v->intvalue;
+    if (const ValueFlow::Value* v = tok->getKnownValue(ValueFlow::Value::ValueType::TOK)) {
+        if (v->tokvalue != tok)
+            return valueFlowGetStrLength(v->tokvalue);
+    }
     return 0;
 }
 

diff --git a/releasenotes.txt b/releasenotes.txt
@@ -2,6 +2,7 @@ Release Notes for Cppcheck 2.12
 
 New checks:
 - uselessOverride finds overriding functions that either duplicate code from or delegate back to the base class implementation
+- knownPointerToBool finds pointer to bool conversions that are always true or false
 
 Improved checking:
 - truncLongCastAssignment and truncLongCastReturn check additional types, including float/double/long double

diff --git a/test/cfg/std.c b/test/cfg/std.c
@@ -4181,7 +4181,6 @@ void uninitvar_tolower(int character)
     // cppcheck-suppress uninitvar
     (void)tolower(c1);
 
-    // cppcheck-suppress unassignedVariable
     int c2;
     // cppcheck-suppress constVariablePointer
     int *pc=&c2;
@@ -4203,7 +4202,6 @@ void uninitvar_toupper(int character)
     // cppcheck-suppress uninitvar
     (void)toupper(c1);
 
-    // cppcheck-suppress unassignedVariable
     int c2;
     // cppcheck-suppress constVariablePointer
     int *pc=&c2;

diff --git a/test/testastutils.cpp b/test/testastutils.cpp
@@ -484,7 +484,7 @@ class TestAstUtils : public TestFixture {
         ASSERT(Result::False == isUsedAsBool("void f() { int i; if (i + 2) {} }", "i +"));
         ASSERT(Result::True == isUsedAsBool("void f() { int i; bool b = i; }", "i ; }"));
         ASSERT(Result::True == isUsedAsBool("void f(bool b); void f() { int i; f(i); }","i )"));
-        ASSERT(Result::True == isUsedAsBool("void f() { int *i; if (*i) {} }", "i )"));
+        ASSERT(Result::False == isUsedAsBool("void f() { int *i; if (*i) {} }", "i )"));
         ASSERT(Result::True == isUsedAsBool("void f() { int *i; if (*i) {} }", "* i )"));
         ASSERT(Result::True == isUsedAsBool("int g(); void h(bool); void f() { h(g()); }", "( ) )"));
         ASSERT(Result::True == isUsedAsBool("int g(int); void h(bool); void f() { h(g(0)); }", "( 0 ) )"));