From c1fc7a22188d80aa46e6eea4426ef6e047b04599 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Mon, 2 Apr 2012 15:45:51 +0200 Subject: [PATCH] Improved CheckOther::checkComparisonOfBoolWithInt and CheckOther::checkComparisonOfBoolExpressionWithInt: - Added support for comparision of bool constant with number constant (-> fixed #1877) and integer variable with boolean expression - Moved a check from checkComparisonOfBoolWithInt to checkComparisonOfBoolExpressionWithInt - Generalized some patterns - Made error message more accurate concnerning the "neither 0 nor 1" part. - Reduced number of Token::Match calls --- lib/checkother.cpp | 149 +++++++++++++++++++++++++-------------------- lib/checkother.h | 8 +-- test/testother.cpp | 136 ++++++++++++++++++++++++----------------- 3 files changed, 166 insertions(+), 127 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cdda9f979..465ece706 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1671,63 +1671,55 @@ void CheckOther::checkComparisonOfBoolWithInt() const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "%var% >|>=|==|!=|<=|< %num%")) { // Comparing variable with number - const Token *varTok = tok; - const Token *numTok = tok->tokAt(2); - if (isBool(symbolDatabase->getVariableFromVarId(varTok->varId())) && // Variable has to be a boolean - ((tok->strAt(1) != "==" && tok->strAt(1) != "!=") || - (MathLib::toLongNumber(numTok->str()) != 0 && 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; - if (isBool(symbolDatabase->getVariableFromVarId(varTok->varId())) && // Variable has to be a boolean - ((tok->strAt(1) != "==" && tok->strAt(1) != "!=") || - (MathLib::toLongNumber(numTok->str()) != 0 && 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; - if (isNonBoolStdType(symbolDatabase->getVariableFromVarId(varTok->varId()))) { // 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); - if (isNonBoolStdType(symbolDatabase->getVariableFromVarId(varTok->varId()))) { // Variable has to be of non-boolean standard type - comparisonOfBoolWithIntError(varTok, constTok->str()); - } - } else if (Token::Match(tok, "%var% >|>=|==|!=|<=|< %var%") && - !Token::Match(tok->tokAt(3), ".|::|(")) { // Comparing two variables, one of them boolean, one of them integer - const Variable* var1 = symbolDatabase->getVariableFromVarId(tok->tokAt(2)->varId()); - const Variable* var2 = symbolDatabase->getVariableFromVarId(tok->varId()); - if (isBool(var1) && isNonBoolStdType(var2)) // Comparing boolean with non-bool standard type - comparisonOfBoolWithIntError(tok, var1->name()); - else if (isNonBoolStdType(var1) && isBool(var2)) // Comparing non-bool standard type with boolean - comparisonOfBoolWithIntError(tok, var2->name()); - } else if (Token::Match(tok, "( ! %var% ==|!= %num% )")) { - const Token *numTok = tok->tokAt(4); - if (numTok && numTok->str() != "0" && numTok->str() != "1") { - comparisonOfBoolWithIntError(numTok, "!"+tok->strAt(2)); - } - } else if (Token::Match(tok, "( %num% ==|!= ! %var% )")) { - const Token *numTok = tok->next(); - if (numTok && numTok->str() != "0" && numTok->str() != "1") { - comparisonOfBoolWithIntError(numTok, "!"+tok->strAt(4)); + if (Token::Match(tok->next(), ">|>=|==|!=|<=|<") && (!tok->previous() || !tok->previous()->isArithmeticalOp()) && (!tok->tokAt(3) || !tok->tokAt(3)->isArithmeticalOp())) { + const Token* const right = tok->tokAt(2); + if ((tok->varId() && right->isNumber()) || (tok->isNumber() && right->varId())) { // Comparing variable with number + const Token* varTok = tok; + const Token* numTok = right; + if (tok->isNumber() && right->varId()) // num with var + std::swap(varTok, numTok); + if (isBool(symbolDatabase->getVariableFromVarId(varTok->varId())) && // Variable has to be a boolean + ((tok->strAt(1) != "==" && tok->strAt(1) != "!=") || + (MathLib::toLongNumber(numTok->str()) != 0 && MathLib::toLongNumber(numTok->str()) != 1))) { // == 0 and != 0 are allowed, for C also == 1 and != 1 + comparisonOfBoolWithIntError(varTok, numTok->str(), tok->strAt(1) == "==" || tok->strAt(1) == "!="); + } + } else if (tok->isBoolean() && right->varId()) { // Comparing boolean constant with variable + if (isNonBoolStdType(symbolDatabase->getVariableFromVarId(right->varId()))) { // Variable has to be of non-boolean standard type + comparisonOfBoolWithIntError(right, tok->str(), false); + } + } else if (tok->varId() && right->isBoolean()) { // Comparing variable with boolean constant + if (isNonBoolStdType(symbolDatabase->getVariableFromVarId(tok->varId()))) { // Variable has to be of non-boolean standard type + comparisonOfBoolWithIntError(tok, right->str(), false); + } + } else if (tok->isNumber() && right->isBoolean()) { // number constant with boolean constant + comparisonOfBoolWithIntError(tok, right->str(), false); + } else if (tok->isBoolean() && right->isNumber()) { // number constant with boolean constant + comparisonOfBoolWithIntError(tok, tok->str(), false); + } else if (tok->varId() && right->varId()) { // Comparing two variables, one of them boolean, one of them integer + const Variable* var1 = symbolDatabase->getVariableFromVarId(right->varId()); + const Variable* var2 = symbolDatabase->getVariableFromVarId(tok->varId()); + if (isBool(var1) && isNonBoolStdType(var2)) // Comparing boolean with non-bool standard type + comparisonOfBoolWithIntError(tok, var1->name(), false); + else if (isNonBoolStdType(var1) && isBool(var2)) // Comparing non-bool standard type with boolean + comparisonOfBoolWithIntError(tok, var2->name(), false); } } } } -void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::string &expression) +void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::string &expression, bool n0o1) { - reportError(tok, Severity::warning, "comparisonOfBoolWithInt", - "Comparison of a boolean with integer that is neither 1 nor 0\n" - "The expression \"" + expression + "\" is of type 'bool' " - "and it is compared against a integer value that is " - "neither 1 nor 0."); + if (n0o1) + reportError(tok, Severity::warning, "comparisonOfBoolWithInt", + "Comparison of a boolean with an integer that is neither 1 nor 0\n" + "The expression \"" + expression + "\" is of type 'bool' " + "and it is compared against a integer value that is " + "neither 1 nor 0."); + else + reportError(tok, Severity::warning, "comparisonOfBoolWithInt", + "Comparison of a boolean with an integer\n" + "The expression \"" + expression + "\" is of type 'bool' " + "and it is compared against a integer value."); } //--------------------------------------------------------------------------- @@ -3220,29 +3212,54 @@ void CheckOther::checkComparisonOfBoolExpressionWithInt() if (!_settings->isEnabled("style")) return; + const SymbolDatabase* symbolDatabase = _tokenizer->getSymbolDatabase(); + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "&&|%oror% %any% ) ==|!=|>|< %num%")) { - const std::string& op = tok->strAt(3); - const std::string& num = tok->strAt(4); - if ((op == "<" || num != "0") && (op == ">" || num != "1")) { - comparisonOfBoolExpressionWithIntError(tok->next()); - } + const Token* numTok = 0; + const Token* opTok = 0; + char op = 0; + if (Token::Match(tok, "&&|%oror% %any% ) >|>=|==|!=|<=|< %any%")) { + numTok = tok->tokAt(4); + opTok = tok->tokAt(3); + if (Token::Match(opTok, "<|>")) + op = opTok->str()[0]; + } else if (Token::Match(tok, "%any% >|>=|==|!=|<=|< ( %any% &&|%oror%")) { + numTok = tok; + opTok = tok->next(); + if (Token::Match(opTok, "<|>")) + op = opTok->str()[0]=='>'?'<':'>'; } - else if (Token::Match(tok, "%num% ==|!=|>|< ( %any% &&|%oror%")) { - const std::string& op = tok->strAt(1); - const std::string& num = tok->str(); - if ((op == ">" || num != "0") && (op == "<" || num != "1")) { - comparisonOfBoolExpressionWithIntError(tok->next()); - } + else if (Token::Match(tok, "! %var% >|>=|==|!=|<=|< %any%")) { + numTok = tok->tokAt(3); + opTok = tok->tokAt(2); + if (Token::Match(opTok, "<|>")) + op = opTok->str()[0]; + } else if (Token::Match(tok, "%any% >|>=|==|!=|<=|< ! %var%")) { + numTok = tok; + opTok = tok->next(); + if (Token::Match(opTok, "<|>")) + op = opTok->str()[0]=='>'?'<':'>'; + } + + if (numTok && opTok) { + if (numTok->isNumber()) { + if (((numTok->str() != "0" && numTok->str() != "1") || !Token::Match(opTok, "!=|==")) && !((op == '<' && numTok->str() == "1") || (op == '>' && numTok->str() == "0"))) + comparisonOfBoolExpressionWithIntError(tok, true); + } else if (isNonBoolStdType(symbolDatabase->getVariableFromVarId(numTok->varId()))) + comparisonOfBoolExpressionWithIntError(tok, false); } } } -void CheckOther::comparisonOfBoolExpressionWithIntError(const Token *tok) +void CheckOther::comparisonOfBoolExpressionWithIntError(const Token *tok, bool n0o1) { - reportError(tok, Severity::warning, "compareBoolExpressionWithInt", - "Comparison of a boolean expression with an integer other than 0 or 1."); + if (n0o1) + reportError(tok, Severity::warning, "compareBoolExpressionWithInt", + "Comparison of a boolean expression with an integer other than 0 or 1."); + else + reportError(tok, Severity::warning, "compareBoolExpressionWithInt", + "Comparison of a boolean expression with an integer."); } diff --git a/lib/checkother.h b/lib/checkother.h index aae99bb8a..e5c8f99bb 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -294,7 +294,7 @@ private: void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len); void incorrectStringBooleanError(const Token *tok, const std::string& string); void incrementBooleanError(const Token *tok); - void comparisonOfBoolWithIntError(const Token *tok, const std::string &expression); + void comparisonOfBoolWithIntError(const Token *tok, const std::string &expression, bool n0o1); 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); @@ -306,7 +306,7 @@ private: void unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive); void unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive); void bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op); - void comparisonOfBoolExpressionWithIntError(const Token *tok); + void comparisonOfBoolExpressionWithIntError(const Token *tok, bool n0o1); void SuspiciousSemicolonError(const Token *tok); void doubleFreeError(const Token *tok, const std::string &varname); void doubleCloseDirError(const Token *tok, const std::string &varname); @@ -353,7 +353,7 @@ private: c.incorrectStringCompareError(0, "substr", "\"Hello World\"", "12"); c.incorrectStringBooleanError(0, "\"Hello World\""); c.incrementBooleanError(0); - c.comparisonOfBoolWithIntError(0, "varname"); + c.comparisonOfBoolWithIntError(0, "varname", true); c.duplicateIfError(0, 0); c.duplicateBranchError(0, 0); c.duplicateExpressionError(0, 0, "&&"); @@ -364,7 +364,7 @@ private: c.unsignedLessThanZeroError(0, "varname", false); c.unsignedPositiveError(0, "varname", false); c.bitwiseOnBooleanError(0, "varname", "&&"); - c.comparisonOfBoolExpressionWithIntError(0); + c.comparisonOfBoolExpressionWithIntError(0, true); c.SuspiciousSemicolonError(0); c.wrongPrintfScanfArgumentsError(0,"printf",3,2); c.invalidScanfArgTypeError(0, "scanf", 1); diff --git a/test/testother.cpp b/test/testother.cpp index 65f235d8f..2fa714958 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -130,7 +130,8 @@ private: TEST_CASE(clarifyCondition4); // ticket #3110 TEST_CASE(bitwiseOnBoolean); // if (bool & bool) - TEST_CASE(comparisonOfBoolExpressionWithInt); + TEST_CASE(comparisonOfBoolExpressionWithInt1); + TEST_CASE(comparisonOfBoolExpressionWithInt2); TEST_CASE(incorrectStringCompare); @@ -140,7 +141,6 @@ private: TEST_CASE(comparisonOfBoolWithInt3); TEST_CASE(comparisonOfBoolWithInt4); TEST_CASE(comparisonOfBoolWithInt5); - TEST_CASE(comparisonOfBoolWithInt6); TEST_CASE(duplicateIf); TEST_CASE(duplicateBranch); @@ -3023,7 +3023,7 @@ private: ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); } - void comparisonOfBoolExpressionWithInt() { + void comparisonOfBoolExpressionWithInt1() { check("void f(int x) {\n" " if ((x && 0x0f)==6)\n" " a++;\n" @@ -3043,7 +3043,8 @@ private: " a++;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n" + "[test.cpp:2]: (warning) Comparison of a boolean with an integer\n", errout.str()); // The second message appears because the code is simplified to "true == 6". Its neither wrong nor necessary. check("void f(int x) {\n" " if ((x || 0x0f)==0)\n" @@ -3161,6 +3162,64 @@ private: ASSERT_EQUALS("", errout.str()); } + void comparisonOfBoolExpressionWithInt2() { + check("void f(int x) {\n" + " if (!x == 10) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (!x != 10) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (x != 10) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (10 == !x) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (10 != !x) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n", errout.str()); + + check("void f(int x, int y) {\n" + " if (y != !x) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer.\n", errout.str()); + + check("void f(int x, bool y) {\n" + " if (y != !x) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (10 != x) {\n" + " printf(\"x not equal to 10\");\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void memsetZeroBytes() { check("void f() {\n" @@ -3641,14 +3700,14 @@ private: " printf(\"foo\");\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an 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 integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an integer\n", errout.str()); check("void f(bool x) {\n" " if (x != 0) {\n" @@ -3668,14 +3727,14 @@ private: " printf(\"foo\");\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an integer that is neither 1 nor 0\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 integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an integer that is neither 1 nor 0\n", errout.str()); check("void f(bool x) {\n" " if (x == 0) {\n" @@ -3691,14 +3750,14 @@ private: " printf(\"foo\");\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an 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 integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an integer\n", errout.str()); check("void f(bool x, bool y) {\n" " if (x == y) {\n" @@ -3721,14 +3780,14 @@ private: " printf(\"foo\");\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an 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 integer that is neither 1 nor 0\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an integer\n", errout.str()); check("void f(bool y) {\n" " if (y == true) {\n" @@ -3736,60 +3795,23 @@ private: " }\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void f(bool y) {\n" + " if (false < 5) {\n" + " printf(\"foo\");\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with an integer\n", errout.str()); } void comparisonOfBoolWithInt4() { - check("void f(int x) {\n" - " if (!x == 10) {\n" - " printf(\"x not equal to 10\");\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); - - check("void f(int x) {\n" - " if (!x != 10) {\n" - " printf(\"x not equal to 10\");\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); - - check("void f(int x) {\n" - " if (x != 10) {\n" - " printf(\"x not equal to 10\");\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if (10 == !x) {\n" - " printf(\"x not equal to 10\");\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); - - check("void f(int x) {\n" - " if (10 != !x) {\n" - " printf(\"x not equal to 10\");\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean with integer that is neither 1 nor 0\n", errout.str()); - - check("void f(int x) {\n" - " if (10 != x) {\n" - " printf(\"x not equal to 10\");\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void comparisonOfBoolWithInt5() { check("void f(int x) {\n" " if (!x == 1) { }\n" "}"); ASSERT_EQUALS("", errout.str()); } - void comparisonOfBoolWithInt6() { + void comparisonOfBoolWithInt5() { check("void SetVisible(int index, bool visible) {\n" " bool (SciTEBase::*ischarforsel)(char ch);\n" " if (visible != GetVisible(index)) { }\n"