From 494d3af3d17028a5553a206c6b4402468ea83bfc Mon Sep 17 00:00:00 2001 From: PKEuS Date: Wed, 5 Oct 2011 20:30:36 +0200 Subject: [PATCH] Fixed #1877 (Be more strict about int vs. bool, part II) --- lib/checkother.cpp | 75 +++++++++++++++++++++++++++--- lib/checkother.h | 2 +- test/testother.cpp | 113 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 178 insertions(+), 12 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index c7ca4c8e3..9ad7b3e88 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -22,7 +22,7 @@ #include "mathlib.h" #include "symboldatabase.h" -#include // std::isupper +#include #include // fabs() #include //--------------------------------------------------------------------------- @@ -1226,14 +1226,77 @@ void CheckOther::checkComparisonOfBoolWithInt() if (!_settings->isEnabled("style")) return; + std::map boolvars; // Contains all declarated standard type variables and indicates whether its a bool or not. for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "( ! %var% ==|!= %num% )")) + if (Token::Match(tok, "[{};(,] %type% %var% [;=,)]") && tok->tokAt(1)->isStandardType()) // Declaration of standard type variable + { + boolvars[tok->tokAt(2)->varId()] = (tok->tokAt(1)->str() == "bool"); + } + else if (Token::Match(tok, "%var% >|>=|==|!=|<=|< %num%")) // Comparing variable with number + { + const Token *varTok = tok; + const Token *numTok = tok->tokAt(2); + std::map::const_iterator iVar = boolvars.find(varTok->varId()); + if (iVar != boolvars.end() && iVar->second && // Variable has to be a boolean + ((tok->tokAt(1)->str() != "==" && tok->tokAt(1)->str() != "!=") || + ((MathLib::toLongNumber(numTok->str()) != 0) && (!code_is_c() || MathLib::toLongNumber(numTok->str()) != 1)))) // == 0 and != 0 are allowed, for C also == 1 and != 1 + { + comparisonOfBoolWithIntError(varTok, numTok->str()); + } + } + else if (Token::Match(tok, "%num% >|>=|==|!=|<=|< %var%")) // Comparing number with variable + { + const Token *varTok = tok->tokAt(2); + const Token *numTok = tok; + std::map::const_iterator iVar = boolvars.find(varTok->varId()); + if (iVar != boolvars.end() && iVar->second && // Variable has to be a boolean + ((tok->tokAt(1)->str() != "==" && tok->tokAt(1)->str() != "!=") || + ((MathLib::toLongNumber(numTok->str()) != 0) && (!code_is_c() || MathLib::toLongNumber(numTok->str()) != 1)))) // == 0 and != 0 are allowed, for C also == 1 and != 1 + { + comparisonOfBoolWithIntError(varTok, numTok->str()); + } + } + else if (Token::Match(tok, "true|false >|>=|==|!=|<=|< %var%")) // Comparing boolean constant with variable + { + const Token *varTok = tok->tokAt(2); + const Token *constTok = tok; + std::map::const_iterator iVar = boolvars.find(varTok->varId()); + if (iVar != boolvars.end() && !iVar->second) // Variable has to be of non-boolean standard type + { + comparisonOfBoolWithIntError(varTok, constTok->str()); + } + } + else if (Token::Match(tok, "%var% >|>=|==|!=|<=|< true|false")) // Comparing variable with boolean constant + { + const Token *varTok = tok; + const Token *constTok = tok->tokAt(2); + std::map::const_iterator iVar = boolvars.find(varTok->varId()); + if (iVar != boolvars.end() && !iVar->second) // Variable has to be of non-boolean standard type + { + comparisonOfBoolWithIntError(varTok, constTok->str()); + } + } + else if (Token::Match(tok, "%var% >|>=|==|!=|<=|< %var%")) // Comparing two variables, one of them boolean, one of them integer + { + const Token *var1Tok = tok->tokAt(2); + const Token *var2Tok = tok; + std::map::const_iterator iVar1 = boolvars.find(var1Tok->varId()); + std::map::const_iterator iVar2 = boolvars.find(var2Tok->varId()); + if (iVar1 != boolvars.end() && iVar2 != boolvars.end()) + { + if (iVar1->second && !iVar2->second) // Comparing boolean with non-bool standard type + comparisonOfBoolWithIntError(var2Tok, var1Tok->str()); + else if (!iVar1->second && iVar2->second) // Comparing non-bool standard type with boolean + comparisonOfBoolWithIntError(var2Tok, var2Tok->str()); + } + } + else if (Token::Match(tok, "( ! %var% ==|!= %num% )")) { const Token *numTok = tok->tokAt(4); if (numTok && numTok->str() != "0") { - comparisonOfBoolWithIntError(numTok, tok->strAt(2)); + comparisonOfBoolWithIntError(numTok, "!"+tok->strAt(2)); } } else if (Token::Match(tok, "( %num% ==|!= ! %var% )")) @@ -1241,17 +1304,17 @@ void CheckOther::checkComparisonOfBoolWithInt() const Token *numTok = tok->tokAt(1); if (numTok && numTok->str() != "0") { - comparisonOfBoolWithIntError(numTok, tok->strAt(4)); + comparisonOfBoolWithIntError(numTok, "!"+tok->strAt(4)); } } } } -void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::string &varname) +void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::string &expression) { reportError(tok, Severity::warning, "comparisonOfBoolWithInt", "Comparison of a boolean with a non-zero integer\n" - "The expression \"!" + varname + "\" is of type 'bool' but is compared against a non-zero 'int'."); + "The expression \"" + expression + "\" is of type 'bool' but is compared against a non-zero 'int'."); } //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index adcdadf9b..0af0d96c9 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -256,7 +256,7 @@ public: void sizeofForNumericParameterError(const Token *tok); void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len); void incrementBooleanError(const Token *tok); - void comparisonOfBoolWithIntError(const Token *tok, const std::string &varname); + void comparisonOfBoolWithIntError(const Token *tok, const std::string &expression); 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); diff --git a/test/testother.cpp b/test/testother.cpp index db00142e7..cfe62ff8b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -61,10 +61,10 @@ private: TEST_CASE(varScope6); TEST_CASE(varScope7); TEST_CASE(varScope8); - TEST_CASE(varScope9); // classes may have extra side-effects + TEST_CASE(varScope9); // classes may have extra side-effects TEST_CASE(varScope10); // Undefined macro FOR - TEST_CASE(varScope11); // #2475 - struct initialization is not inner scope - TEST_CASE(varScope12); // variable usage in inner loop + TEST_CASE(varScope11); // #2475 - struct initialization is not inner scope + TEST_CASE(varScope12); // variable usage in inner loop TEST_CASE(oldStylePointerCast); @@ -126,7 +126,10 @@ private: TEST_CASE(incorrectStringCompare); TEST_CASE(incrementBoolean); - TEST_CASE(comparisonOfBoolWithInt); + TEST_CASE(comparisonOfBoolWithInt1); + TEST_CASE(comparisonOfBoolWithInt2); + TEST_CASE(comparisonOfBoolWithInt3); + TEST_CASE(comparisonOfBoolWithInt4); TEST_CASE(duplicateIf); TEST_CASE(duplicateBranch); @@ -2956,7 +2959,107 @@ private: ASSERT_EQUALS("", errout.str()); } - void comparisonOfBoolWithInt() + void comparisonOfBoolWithInt1() + { + check("void f(bool x) {\n" + " if (x < 10) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(bool x) {\n" + " if (10 >= x) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(bool x) {\n" + " if (x != 0) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(bool x) {\n" + " if (x != 10) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(bool x) {\n" + " if (x == 10) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(bool x) {\n" + " if (x == 0) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void comparisonOfBoolWithInt2() + { + check("void f(bool x, int y) {\n" + " if (x == y) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(int x, bool y) {\n" + " if (x == y) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(bool x, bool y) {\n" + " if (x == y) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(bool x, fooClass y) {\n" + " if (x == y) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void comparisonOfBoolWithInt3() + { + check("void f(int y) {\n" + " if (y > false) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(int y) {\n" + " if (true == y) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with a non-zero integer\n", errout.str()); + + check("void f(bool y) {\n" + " if (y == true) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void comparisonOfBoolWithInt4() { check("void f(int x) {\n" " if (!x == 10) {\n"