From a3fbad50cb2d21d88f51b105a73ef89773dfda6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 16 Jul 2015 20:17:57 +0200 Subject: [PATCH] Fixed #4842 (condition is always true (variable is assigned constant value and then used in condition)) --- lib/checkcondition.cpp | 37 +++++++++++++++++++++++++++++++++++++ lib/checkcondition.h | 10 +++++++++- lib/valueflow.cpp | 12 ++++++++++++ test/testcondition.cpp | 11 +++++++++++ test/testvalueflow.cpp | 9 +++++++++ 5 files changed, 78 insertions(+), 1 deletion(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 547a42d85..3618230a9 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -930,3 +930,40 @@ void CheckCondition::clarifyConditionError(const Token *tok, bool assign, bool b "clarifyCondition", errmsg); } + + +void CheckCondition::alwaysTrueFalse() +{ + if (!_settings->isEnabled("style")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (!Token::Match(tok, "%comp%|!")) + continue; + if (tok->link()) // don't write false positives when templates are used + continue; + if (tok->values.size() != 1U) + continue; + if (tok->values.front().valueKind != ValueFlow::Value::Known) + continue; + + if (tok->astParent() && Token::Match(tok->astParent()->previous(), "%name% (")) + alwaysTrueFalseError(tok, tok->values.front().intvalue != 0); + } + } +} + +void CheckCondition::alwaysTrueFalseError(const Token *tok, bool knownResult) +{ + const std::string expr = tok ? tok->expressionString() : std::string("x"); + + reportError(tok, + Severity::style, + "knownConditionTrueFalse", + "Condition " + expr + " is always " + (knownResult ? "true" : "false")); +} diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 1570a4e3e..9b5300260 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -59,6 +59,7 @@ public: checkCondition.oppositeInnerCondition(); checkCondition.checkIncorrectLogicOperator(); checkCondition.checkModuloAlwaysTrueFalse(); + checkCondition.alwaysTrueFalse(); } /** mismatching assignment / comparison */ @@ -93,6 +94,9 @@ public: /** @brief Suspicious condition (assignment+comparison) */ void clarifyCondition(); + /** @brief Condition is always true/false */ + void alwaysTrueFalse(); + private: bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set &constFunctions) const; @@ -123,6 +127,8 @@ private: void clarifyConditionError(const Token *tok, bool assign, bool boolop); + void alwaysTrueFalseError(const Token *tok, bool knownResult); + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckCondition c(0, settings, errorLogger); @@ -136,6 +142,7 @@ private: c.redundantConditionError(0, "If x > 11 the condition x > 10 is always true."); c.moduloAlwaysTrueFalseError(0, "1"); c.clarifyConditionError(0, true, false); + c.alwaysTrueFalseError(0, true); } static std::string myName() { @@ -152,7 +159,8 @@ private: "- Find dead code which is inaccessible due to the counter-conditions check in nested if statements\n" "- condition that is always true/false\n" "- mutual exclusion over || always evaluating to true\n" - "- Comparisons of modulo results that are always true/false.\n"; + "- Comparisons of modulo results that are always true/false.\n" + "- Known variable values => condition is always true/false\n"; } }; /// @} diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 377e82bd5..4f77b8c63 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -407,6 +407,18 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value) } } + // ! + else if (parent->str() == "!") { + std::list::const_iterator it; + for (it = tok->values.begin(); it != tok->values.end(); ++it) { + if (it->tokvalue) + continue; + ValueFlow::Value v(*it); + v.intvalue = !v.intvalue; + setTokenValue(parent, v); + } + } + // Array element else if (parent->str() == "[" && parent->astOperand1() && parent->astOperand2()) { std::list::const_iterator value1, value2; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 199e193db..2e1862877 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -62,6 +62,8 @@ private: TEST_CASE(clarifyCondition4); // ticket #3110 TEST_CASE(clarifyCondition5); // #3609 CWinTraits.. TEST_CASE(clarifyCondition6); // #3818 + + TEST_CASE(alwaysTrue); } void check(const char code[], bool validate=true, const char* filename = "test.cpp") { @@ -1487,6 +1489,15 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void alwaysTrue() { + check("void f() {\n" // #4842 + " int x = 0;\n" + " if (a) { return; }\n" // <- this is just here to fool simplifyKnownVariabels + " if (!x) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Condition !x is always true\n", errout.str()); + } }; REGISTER_TEST(TestCondition) diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index e59b27673..47d030fed 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -265,6 +265,15 @@ private: ASSERT_EQUALS(2, values.front().intvalue); ASSERT_EQUALS(3, values.back().intvalue); + // ! + code = "void f(int x) {\n" + " a = !x;\n" + " if (x==0) {}\n" + "}"; + values = tokenValues(code,"!"); + ASSERT_EQUALS(1U, values.size()); + ASSERT_EQUALS(1, values.back().intvalue); + // function call => calculation code = "void f(int x) {\n" " a = x + 8;\n"