From 50a94885ce0a8fe88b5753398148a1e02c44843d Mon Sep 17 00:00:00 2001 From: PKEuS Date: Fri, 28 Mar 2014 14:55:17 +0100 Subject: [PATCH] Fixed oppositeInnerCondition check: - Resolved false positives #4170 and #4186, as well as numerous other potential false positives - Improved message to point to both locations - Inner condition could also be a while loop; Outer if could also be 'else if' - Made the check non-experimental again (#3645) --- lib/checkother.cpp | 82 +++++++++++++++++++++++----------------------- lib/checkother.h | 4 +-- test/testother.cpp | 51 ++++++++++++++++++++++++---- 3 files changed, 88 insertions(+), 49 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index f6068b213..25a181e76 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3377,59 +3377,56 @@ void CheckOther::incompleteArrayFillError(const Token* tok, const std::string& b void CheckOther::oppositeInnerCondition() { - // FIXME: This check is experimental because of #4170 and #4186. Fix those tickets and remove the "experimental". - if (!_settings->isEnabled("warning") || !_settings->inconclusive || !_settings->experimental) + if (!_settings->isEnabled("warning") || !_settings->inconclusive) return; const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - const Token* const toke = scope->classDef; + if (scope->type != Scope::eIf && scope->type != Scope::eElseIf) + continue; + const Token *op1Tok, *op2Tok; + op1Tok = scope->classDef->tokAt(2); + op2Tok = scope->classDef->tokAt(4); - if (scope->type == Scope::eIf && toke) { + if (scope->classDef->strAt(6) == "{") { - const Token *op1Tok, *op2Tok; - op1Tok = scope->classDef->tokAt(2); - op2Tok = scope->classDef->tokAt(4); + const char *oppositeCondition = nullptr; - if (scope->classDef->strAt(6) == "{") { + if (scope->classDef->strAt(3) == "==") + oppositeCondition = "if|while ( %any% !=|<|>|<=|>= %any% )"; + else if (scope->classDef->strAt(3) == "!=") + oppositeCondition = "if|while ( %any% ==|>=|<= %any% )"; + else if (scope->classDef->strAt(3) == "<") + oppositeCondition = "if|while ( %any% >|>=|== %any% )"; + else if (scope->classDef->strAt(3) == "<=") + oppositeCondition = "if|while ( %any% > %any% )"; + else if (scope->classDef->strAt(3) == ">") + oppositeCondition = "if|while ( %any% <|<=|== %any% )"; + else if (scope->classDef->strAt(3) == ">=") + oppositeCondition = "if|while ( %any% < %any% )"; - const char *oppositeCondition = nullptr; + if (oppositeCondition) { + int flag = 0; - if (scope->classDef->strAt(3) == "==") - oppositeCondition = "if ( %any% !=|<|>|<=|>= %any% )"; - else if (scope->classDef->strAt(3) == "!=") - oppositeCondition = "if ( %any% ==|>=|<= %any% )"; - else if (scope->classDef->strAt(3) == "<") - oppositeCondition = "if ( %any% >|>=|== %any% )"; - else if (scope->classDef->strAt(3) == "<=") - oppositeCondition = "if ( %any% > %any% )"; - else if (scope->classDef->strAt(3) == ">") - oppositeCondition = "if ( %any% <|<=|== %any% )"; - else if (scope->classDef->strAt(3) == ">=") - oppositeCondition = "if ( %any% < %any% )"; + for (const Token* tok = scope->classStart; tok != scope->classEnd && flag == 0; tok = tok->next()) { + if ((tok->str() == op1Tok->str() || tok->str() == op2Tok->str()) && (tok->next()->isAssignmentOp() || tok->next()->type() == Token::eIncDecOp)) + break; - if (oppositeCondition) { - int flag = 0; - - for (const Token* tok = scope->classStart; tok != scope->classEnd && flag == 0; tok = tok->next()) { - if ((tok->str() == op1Tok->str() || tok->str() == op2Tok->str()) && tok->strAt(1) == "=") - break; - else if (Token::Match(tok, "%any% ( %any% )")) { - if ((tok->strAt(2) == op1Tok->str() || tok->strAt(2) == op2Tok->str())) + if (Token::Match(tok, oppositeCondition)) { + if ((tok->strAt(2) == op1Tok->str() && tok->strAt(4) == op2Tok->str()) || (tok->strAt(2) == op2Tok->str() && tok->strAt(4) == op1Tok->str())) + oppositeInnerConditionError(scope->classDef, tok); + } else if (Token::Match(tok, "%any% (")) { + if (tok->function()) { // Check if it is a member function. If yes: bailout (TODO: this causes false negatives, since we do not check of which function this is a member + const Function* func = tok->function(); + if (!func->isConst && (func->access == Private || func->access == Protected || func->access == Public)) break; - } else if (Token::Match(tok, "%any% ( %any% , %any%")) { - for (const Token* tok2 = tok->next(); tok2 != tok->linkAt(1); tok2 = tok2->next()) { - if (tok2->str() == op1Tok->str()) { - flag = 1; - break; - } - } - } else if (Token::Match(tok, oppositeCondition)) { - if ((tok->strAt(2) == op1Tok->str() && tok->strAt(4) == op2Tok->str()) || (tok->strAt(2) == op2Tok->str() && tok->strAt(4) == op1Tok->str())) - oppositeInnerConditionError(toke); } + if (op1Tok->varId() && Token::findmatch(tok->next(), "%varid%", tok->linkAt(1), op1Tok->varId()) != nullptr) + break; + if (op2Tok->varId() && Token::findmatch(tok->next(), "%varid%", tok->linkAt(1), op2Tok->varId()) != nullptr) + break; } } } @@ -3437,9 +3434,12 @@ void CheckOther::oppositeInnerCondition() } } -void CheckOther::oppositeInnerConditionError(const Token *tok) +void CheckOther::oppositeInnerConditionError(const Token *tok1, const Token* tok2) { - reportError(tok, Severity::warning, "oppositeInnerCondition", "Opposite conditions in nested 'if' blocks lead to a dead code block.", true); + std::list callstack; + callstack.push_back(tok1); + callstack.push_back(tok2); + reportError(callstack, Severity::warning, "oppositeInnerCondition", "Opposite conditions in nested 'if' blocks lead to a dead code block.", true); } diff --git a/lib/checkother.h b/lib/checkother.h index 8b184d413..5909857b9 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -273,7 +273,7 @@ private: void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName); void checkPipeParameterSizeError(const Token *tok, const std::string &strVarName, const std::string &strDim); - void oppositeInnerConditionError(const Token *tok); + void oppositeInnerConditionError(const Token *tok1, const Token* tok2); void clarifyCalculationError(const Token *tok, const std::string &op); void clarifyConditionError(const Token *tok, bool assign, bool boolop); void clarifyStatementError(const Token* tok); @@ -357,7 +357,7 @@ private: // style/warning c.checkComparisonFunctionIsAlwaysTrueOrFalseError(0,"isless","varName",false); c.checkCastIntToCharAndBackError(0,"func_name"); - c.oppositeInnerConditionError(0); + c.oppositeInnerConditionError(0, 0); c.cstyleCastError(0); c.passedByValueError(0, "parametername"); c.constStatementError(0, "type"); diff --git a/test/testother.cpp b/test/testother.cpp index 0a6089d5d..c4fc915c6 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -297,13 +297,52 @@ private: void oppositeInnerCondition() { - check("void foo(int a, int b)\n" - "{\n" + check("void foo(int a, int b) {\n" " if(a==b)\n" " if(a!=b)\n" " cout << a;\n" - "}", "test.cpp", true, true); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning, inconclusive) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + + check("void foo(int a) {\n" + " if(a >= 50) {\n" + " if(a < 50)\n" + " cout << a;\n" + " else\n" + " cout << 100;\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning, inconclusive) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + + // #4186 + check("void foo(int a) {\n" + " if(a >= 50) {\n" + " if(a > 50)\n" + " cout << a;\n" + " else\n" + " cout << 100;\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // 4170 + check("class foo {\n" + " void bar() {\n" + " if (tok == '(') {\n" + " next();\n" + " if (tok == ',') {\n" + " next();\n" + " if (tok != ',') {\n" + " op->reg2 = asm_parse_reg();\n" + " }\n" + " skip(',');\n" + " }\n" + " }\n" + " }\n" + " void next();\n" + " const char *tok;\n" + "};"); + ASSERT_EQUALS("", errout.str()); check("void foo(int i)\n" "{\n" @@ -313,7 +352,7 @@ private: " cout << a;\n" " }\n" " }\n" - "}", "test.cpp", true, true); + "}"); ASSERT_EQUALS("", errout.str()); @@ -328,7 +367,7 @@ private: " if(i<5){\n" " }\n" " }\n" - "}", "test.cpp", true, true); + "}"); ASSERT_EQUALS("", errout.str()); }