From 06a92e898140408e7be81c58c2e64e3b77152a14 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Fri, 29 Aug 2014 17:06:46 +0200 Subject: [PATCH] Moved several condition checks from checkOther to checkCondition (former checkAssignIf) --- lib/checkassignif.cpp | 332 ------ lib/checkcondition.cpp | 855 +++++++++++++++ lib/{checkassignif.h => checkcondition.h} | 72 +- lib/checkother.cpp | 526 +-------- lib/checkother.h | 34 +- lib/cppcheck.vcxproj | 4 +- lib/cppcheck.vcxproj.filters | 12 +- test/testassignif.cpp | 395 ------- test/testcondition.cpp | 1188 +++++++++++++++++++++ test/testother.cpp | 822 +------------- test/testrunner.vcxproj | 2 +- test/testrunner.vcxproj.filters | 6 +- 12 files changed, 2117 insertions(+), 2131 deletions(-) delete mode 100644 lib/checkassignif.cpp create mode 100644 lib/checkcondition.cpp rename lib/{checkassignif.h => checkcondition.h} (57%) delete mode 100644 test/testassignif.cpp create mode 100644 test/testcondition.cpp diff --git a/lib/checkassignif.cpp b/lib/checkassignif.cpp deleted file mode 100644 index 33f012a9a..000000000 --- a/lib/checkassignif.cpp +++ /dev/null @@ -1,332 +0,0 @@ -/* - * Cppcheck - A tool for static C/C++ code analysis - * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - -//--------------------------------------------------------------------------- -// Check for assignment / condition mismatches -//--------------------------------------------------------------------------- - -#include "checkassignif.h" -#include "symboldatabase.h" - -//--------------------------------------------------------------------------- - -// Register this check class (by creating a static instance of it) -namespace { - CheckAssignIf instance; -} - - -void CheckAssignIf::assignIf() -{ - if (!_settings->isEnabled("style")) - return; - - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (tok->str() != "=") - continue; - - if (Token::Match(tok->tokAt(-2), "[;{}] %var% =")) { - const Variable *var = tok->previous()->variable(); - if (var == 0) - continue; - - char bitop = '\0'; - MathLib::bigint num = 0; - - if (Token::Match(tok->next(), "%num% [&|]")) { - bitop = tok->strAt(2).at(0); - num = MathLib::toLongNumber(tok->next()->str()); - } else { - const Token *endToken = Token::findsimplematch(tok, ";"); - - // Casting address - if (endToken && Token::Match(endToken->tokAt(-4), "* ) & %any% ;")) - endToken = nullptr; - - if (endToken && Token::Match(endToken->tokAt(-2), "[&|] %num% ;")) { - bitop = endToken->strAt(-2).at(0); - num = MathLib::toLongNumber(endToken->previous()->str()); - } - } - - if (bitop == '\0') - continue; - - if (num < 0 && bitop == '|') - continue; - - assignIfParseScope(tok, tok->tokAt(4), var->declarationId(), var->isLocal(), bitop, num); - } - } -} - -/** parse scopes recursively */ -bool CheckAssignIf::assignIfParseScope(const Token * const assignTok, - const Token * const startTok, - const unsigned int varid, - const bool islocal, - const char bitop, - const MathLib::bigint num) -{ - bool ret = false; - - for (const Token *tok2 = startTok; tok2; tok2 = tok2->next()) { - if (Token::Match(tok2->tokAt(2), "%varid% %cop% %num% ;", varid) && tok2->strAt(3) == std::string(1U, bitop)) { - const MathLib::bigint num2 = MathLib::toLongNumber(tok2->strAt(4)); - if ((bitop == '&') && (0 == (num & num2))) - mismatchingBitAndError(assignTok, num, tok2, num2); - } - if (Token::Match(tok2, "%varid% =", varid)) { - if (Token::Match(tok2->tokAt(2), "%varid% %cop% %num% ;", varid) && tok2->strAt(3) == std::string(1U, bitop)) { - const MathLib::bigint num2 = MathLib::toLongNumber(tok2->strAt(4)); - if ((bitop == '&') && (0 == (num & num2))) - mismatchingBitAndError(assignTok, num, tok2, num2); - } - return true; - } - if (Token::Match(tok2, "++|-- %varid%", varid) || Token::Match(tok2, "%varid% ++|--", varid)) - return true; - if (Token::Match(tok2, "[(,] &| %varid% [,)]", varid)) { - unsigned int argumentNumber = 0; - const Token *ftok; - for (ftok = tok2; ftok && ftok->str() != "("; ftok = ftok->previous()) { - if (ftok->str() == ")") - ftok = ftok->link(); - else if (ftok->str() == ",") - argumentNumber++; - } - ftok = ftok ? ftok->previous() : nullptr; - if (!(ftok && ftok->function())) - return true; - const Variable *par = ftok->function()->getArgumentVar(argumentNumber); - if (par == nullptr || par->isReference() || par->isPointer()) - return true; - } - if (tok2->str() == "}") - return false; - if (Token::Match(tok2, "break|continue|return")) - ret = true; - if (ret && tok2->str() == ";") - return false; - if (!islocal && Token::Match(tok2, "%var% (") && !Token::simpleMatch(tok2->next()->link(), ") {")) - return true; - if (Token::Match(tok2, "if|while (")) { - if (!islocal && tok2->str() == "while") - continue; - - // parse condition - const Token * const end = tok2->next()->link(); - for (; tok2 != end; tok2 = tok2->next()) { - if (Token::Match(tok2, "[(,] &| %varid% [,)]", varid)) { - return true; - } - if (Token::Match(tok2,"&&|%oror%|( %varid% %any% %num% &&|%oror%|)", varid)) { - const Token *vartok = tok2->next(); - const std::string& op(vartok->strAt(1)); - const MathLib::bigint num2 = MathLib::toLongNumber(vartok->strAt(2)); - const std::string condition(vartok->str() + op + vartok->strAt(2)); - if (op == "==" && (num & num2) != ((bitop=='&') ? num2 : num)) - assignIfError(assignTok, tok2, condition, false); - else if (op == "!=" && (num & num2) != ((bitop=='&') ? num2 : num)) - assignIfError(assignTok, tok2, condition, true); - } - } - - bool ret1 = assignIfParseScope(assignTok, end->tokAt(2), varid, islocal, bitop, num); - bool ret2 = false; - if (Token::simpleMatch(end->next()->link(), "} else {")) - ret2 = assignIfParseScope(assignTok, end->next()->link()->tokAt(3), varid, islocal, bitop, num); - if (ret1 || ret2) - return true; - } - } - return false; -} - -void CheckAssignIf::assignIfError(const Token *tok1, const Token *tok2, const std::string &condition, bool result) -{ - std::list locations; - locations.push_back(tok1); - locations.push_back(tok2); - - reportError(locations, - Severity::style, - "assignIfError", - "Mismatching assignment and comparison, comparison '" + condition + "' is always " + std::string(result ? "true" : "false") + "."); -} - - -void CheckAssignIf::mismatchingBitAndError(const Token *tok1, const MathLib::bigint num1, const Token *tok2, const MathLib::bigint num2) -{ - std::list locations; - locations.push_back(tok1); - locations.push_back(tok2); - - std::ostringstream msg; - msg << "Mismatching bitmasks. Result is always 0 (" - << "X = Y & 0x" << std::hex << num1 << "; Z = X & 0x" << std::hex << num2 << "; => Z=0)."; - - reportError(locations, - Severity::style, - "mismatchingBitAnd", - msg.str()); -} - - -static void getnumchildren(const Token *tok, std::list &numchildren) -{ - if (tok->astOperand1() && tok->astOperand1()->isNumber()) - numchildren.push_back(MathLib::toLongNumber(tok->astOperand1()->str())); - else if (tok->astOperand1() && tok->str() == tok->astOperand1()->str()) - getnumchildren(tok->astOperand1(), numchildren); - if (tok->astOperand2() && tok->astOperand2()->isNumber()) - numchildren.push_back(MathLib::toLongNumber(tok->astOperand2()->str())); - else if (tok->astOperand2() && tok->str() == tok->astOperand2()->str()) - getnumchildren(tok->astOperand2(), numchildren); -} - -void CheckAssignIf::comparison() -{ - if (!_settings->isEnabled("style")) - return; - - // Experimental code based on AST - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "==|!=")) { - const Token *expr1 = tok->astOperand1(); - const Token *expr2 = tok->astOperand2(); - if (!expr1 || !expr2) - continue; - if (expr1->isNumber()) - std::swap(expr1,expr2); - if (!expr2->isNumber()) - continue; - const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str()); - if (num2 < 0) - continue; - if (!Token::Match(expr1,"[&|]")) - continue; - std::list numbers; - getnumchildren(expr1, numbers); - for (std::list::const_iterator num = numbers.begin(); num != numbers.end(); ++num) { - const MathLib::bigint num1 = *num; - if (num1 < 0) - continue; - if ((expr1->str() == "&" && (num1 & num2) != num2) || - (expr1->str() == "|" && (num1 | num2) != num2)) { - const std::string& op(tok->str()); - comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true); - } - } - } - } -} - -void CheckAssignIf::comparisonError(const Token *tok, const std::string &bitop, MathLib::bigint value1, const std::string &op, MathLib::bigint value2, bool result) -{ - std::ostringstream expression; - expression << std::hex << "(X " << bitop << " 0x" << value1 << ") " << op << " 0x" << value2; - - const std::string errmsg("Expression '" + expression.str() + "' is always " + (result?"true":"false") + ".\n" - "The expression '" + expression.str() + "' is always " + (result?"true":"false") + - ". Check carefully constants and operators used, these errors might be hard to " - "spot sometimes. In case of complex expression it might help to split it to " - "separate expressions."); - - reportError(tok, Severity::style, "comparisonError", errmsg); -} - -extern bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions); - -static bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set &constFunctions) -{ - if (!cond1 || !cond2) - return false; - - // same expressions - if (isSameExpression(cond1,cond2,constFunctions)) - return true; - - // bitwise overlap for example 'x&7' and 'x==1' - if (cond1->str() == "&" && cond1->astOperand1() && cond2->astOperand2()) { - const Token *expr1 = cond1->astOperand1(); - const Token *num1 = cond1->astOperand2(); - if (!num1) // unary operator& - return false; - if (!num1->isNumber()) - std::swap(expr1,num1); - if (!num1->isNumber() || MathLib::isNegative(num1->str())) - return false; - - if (!Token::Match(cond2, "&|==") || !cond2->astOperand1() || !cond2->astOperand2()) - return false; - const Token *expr2 = cond2->astOperand1(); - const Token *num2 = cond2->astOperand2(); - if (!num2->isNumber()) - std::swap(expr2,num2); - if (!num2->isNumber() || MathLib::isNegative(num2->str())) - return false; - - if (!isSameExpression(expr1,expr2,constFunctions)) - return false; - - const MathLib::bigint value1 = MathLib::toLongNumber(num1->str()); - const MathLib::bigint value2 = MathLib::toLongNumber(num2->str()); - return ((value1 & value2) == value2); - } - return false; -} - - -void CheckAssignIf::multiCondition() -{ - if (!_settings->isEnabled("style")) - return; - - const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); - - for (std::list::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) { - if (i->type != Scope::eIf || !Token::simpleMatch(i->classDef, "if (")) - continue; - - const Token * const cond1 = i->classDef->next()->astOperand2(); - - const Token * tok2 = i->classDef->next(); - while (tok2) { - tok2 = tok2->link(); - if (!Token::simpleMatch(tok2, ") {")) - break; - tok2 = tok2->linkAt(1); - if (!Token::simpleMatch(tok2, "} else { if (")) - break; - tok2 = tok2->tokAt(4); - - if (isOverlappingCond(cond1, tok2->astOperand2(), _settings->library.functionpure)) - multiConditionError(tok2, cond1->linenr()); - } - } -} - -void CheckAssignIf::multiConditionError(const Token *tok, unsigned int line1) -{ - std::ostringstream errmsg; - errmsg << "Expression is always false because 'else if' condition matches previous condition at line " - << line1 << "."; - - reportError(tok, Severity::style, "multiCondition", errmsg.str()); -} diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp new file mode 100644 index 000000000..83c606810 --- /dev/null +++ b/lib/checkcondition.cpp @@ -0,0 +1,855 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +//--------------------------------------------------------------------------- +// Check for condition mismatches +//--------------------------------------------------------------------------- + +#include "checkcondition.h" +#include "checkother.h" +#include "symboldatabase.h" + +//--------------------------------------------------------------------------- + +// Register this check class (by creating a static instance of it) +namespace { + CheckCondition instance; +} + + +void CheckCondition::assignIf() +{ + if (!_settings->isEnabled("style")) + return; + + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (tok->str() != "=") + continue; + + if (Token::Match(tok->tokAt(-2), "[;{}] %var% =")) { + const Variable *var = tok->previous()->variable(); + if (var == 0) + continue; + + char bitop = '\0'; + MathLib::bigint num = 0; + + if (Token::Match(tok->next(), "%num% [&|]")) { + bitop = tok->strAt(2).at(0); + num = MathLib::toLongNumber(tok->next()->str()); + } else { + const Token *endToken = Token::findsimplematch(tok, ";"); + + // Casting address + if (endToken && Token::Match(endToken->tokAt(-4), "* ) & %any% ;")) + endToken = nullptr; + + if (endToken && Token::Match(endToken->tokAt(-2), "[&|] %num% ;")) { + bitop = endToken->strAt(-2).at(0); + num = MathLib::toLongNumber(endToken->previous()->str()); + } + } + + if (bitop == '\0') + continue; + + if (num < 0 && bitop == '|') + continue; + + assignIfParseScope(tok, tok->tokAt(4), var->declarationId(), var->isLocal(), bitop, num); + } + } +} + +/** parse scopes recursively */ +bool CheckCondition::assignIfParseScope(const Token * const assignTok, + const Token * const startTok, + const unsigned int varid, + const bool islocal, + const char bitop, + const MathLib::bigint num) +{ + bool ret = false; + + for (const Token *tok2 = startTok; tok2; tok2 = tok2->next()) { + if (Token::Match(tok2->tokAt(2), "%varid% %cop% %num% ;", varid) && tok2->strAt(3) == std::string(1U, bitop)) { + const MathLib::bigint num2 = MathLib::toLongNumber(tok2->strAt(4)); + if ((bitop == '&') && (0 == (num & num2))) + mismatchingBitAndError(assignTok, num, tok2, num2); + } + if (Token::Match(tok2, "%varid% =", varid)) { + if (Token::Match(tok2->tokAt(2), "%varid% %cop% %num% ;", varid) && tok2->strAt(3) == std::string(1U, bitop)) { + const MathLib::bigint num2 = MathLib::toLongNumber(tok2->strAt(4)); + if ((bitop == '&') && (0 == (num & num2))) + mismatchingBitAndError(assignTok, num, tok2, num2); + } + return true; + } + if (Token::Match(tok2, "++|-- %varid%", varid) || Token::Match(tok2, "%varid% ++|--", varid)) + return true; + if (Token::Match(tok2, "[(,] &| %varid% [,)]", varid)) { + unsigned int argumentNumber = 0; + const Token *ftok; + for (ftok = tok2; ftok && ftok->str() != "("; ftok = ftok->previous()) { + if (ftok->str() == ")") + ftok = ftok->link(); + else if (ftok->str() == ",") + argumentNumber++; + } + ftok = ftok ? ftok->previous() : nullptr; + if (!(ftok && ftok->function())) + return true; + const Variable *par = ftok->function()->getArgumentVar(argumentNumber); + if (par == nullptr || par->isReference() || par->isPointer()) + return true; + } + if (tok2->str() == "}") + return false; + if (Token::Match(tok2, "break|continue|return")) + ret = true; + if (ret && tok2->str() == ";") + return false; + if (!islocal && Token::Match(tok2, "%var% (") && !Token::simpleMatch(tok2->next()->link(), ") {")) + return true; + if (Token::Match(tok2, "if|while (")) { + if (!islocal && tok2->str() == "while") + continue; + + // parse condition + const Token * const end = tok2->next()->link(); + for (; tok2 != end; tok2 = tok2->next()) { + if (Token::Match(tok2, "[(,] &| %varid% [,)]", varid)) { + return true; + } + if (Token::Match(tok2,"&&|%oror%|( %varid% %any% %num% &&|%oror%|)", varid)) { + const Token *vartok = tok2->next(); + const std::string& op(vartok->strAt(1)); + const MathLib::bigint num2 = MathLib::toLongNumber(vartok->strAt(2)); + const std::string condition(vartok->str() + op + vartok->strAt(2)); + if (op == "==" && (num & num2) != ((bitop=='&') ? num2 : num)) + assignIfError(assignTok, tok2, condition, false); + else if (op == "!=" && (num & num2) != ((bitop=='&') ? num2 : num)) + assignIfError(assignTok, tok2, condition, true); + } + } + + bool ret1 = assignIfParseScope(assignTok, end->tokAt(2), varid, islocal, bitop, num); + bool ret2 = false; + if (Token::simpleMatch(end->next()->link(), "} else {")) + ret2 = assignIfParseScope(assignTok, end->next()->link()->tokAt(3), varid, islocal, bitop, num); + if (ret1 || ret2) + return true; + } + } + return false; +} + +void CheckCondition::assignIfError(const Token *tok1, const Token *tok2, const std::string &condition, bool result) +{ + std::list locations; + locations.push_back(tok1); + locations.push_back(tok2); + + reportError(locations, + Severity::style, + "assignIfError", + "Mismatching assignment and comparison, comparison '" + condition + "' is always " + std::string(result ? "true" : "false") + "."); +} + + +void CheckCondition::mismatchingBitAndError(const Token *tok1, const MathLib::bigint num1, const Token *tok2, const MathLib::bigint num2) +{ + std::list locations; + locations.push_back(tok1); + locations.push_back(tok2); + + std::ostringstream msg; + msg << "Mismatching bitmasks. Result is always 0 (" + << "X = Y & 0x" << std::hex << num1 << "; Z = X & 0x" << std::hex << num2 << "; => Z=0)."; + + reportError(locations, + Severity::style, + "mismatchingBitAnd", + msg.str()); +} + + +static void getnumchildren(const Token *tok, std::list &numchildren) +{ + if (tok->astOperand1() && tok->astOperand1()->isNumber()) + numchildren.push_back(MathLib::toLongNumber(tok->astOperand1()->str())); + else if (tok->astOperand1() && tok->str() == tok->astOperand1()->str()) + getnumchildren(tok->astOperand1(), numchildren); + if (tok->astOperand2() && tok->astOperand2()->isNumber()) + numchildren.push_back(MathLib::toLongNumber(tok->astOperand2()->str())); + else if (tok->astOperand2() && tok->str() == tok->astOperand2()->str()) + getnumchildren(tok->astOperand2(), numchildren); +} + +void CheckCondition::comparison() +{ + if (!_settings->isEnabled("style")) + return; + + // Experimental code based on AST + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "==|!=")) { + const Token *expr1 = tok->astOperand1(); + const Token *expr2 = tok->astOperand2(); + if (!expr1 || !expr2) + continue; + if (expr1->isNumber()) + std::swap(expr1,expr2); + if (!expr2->isNumber()) + continue; + const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str()); + if (num2 < 0) + continue; + if (!Token::Match(expr1,"[&|]")) + continue; + std::list numbers; + getnumchildren(expr1, numbers); + for (std::list::const_iterator num = numbers.begin(); num != numbers.end(); ++num) { + const MathLib::bigint num1 = *num; + if (num1 < 0) + continue; + if ((expr1->str() == "&" && (num1 & num2) != num2) || + (expr1->str() == "|" && (num1 | num2) != num2)) { + const std::string& op(tok->str()); + comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true); + } + } + } + } +} + +void CheckCondition::comparisonError(const Token *tok, const std::string &bitop, MathLib::bigint value1, const std::string &op, MathLib::bigint value2, bool result) +{ + std::ostringstream expression; + expression << std::hex << "(X " << bitop << " 0x" << value1 << ") " << op << " 0x" << value2; + + const std::string errmsg("Expression '" + expression.str() + "' is always " + (result?"true":"false") + ".\n" + "The expression '" + expression.str() + "' is always " + (result?"true":"false") + + ". Check carefully constants and operators used, these errors might be hard to " + "spot sometimes. In case of complex expression it might help to split it to " + "separate expressions."); + + reportError(tok, Severity::style, "comparisonError", errmsg); +} + +extern bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions); + +static bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set &constFunctions) +{ + if (!cond1 || !cond2) + return false; + + // same expressions + if (isSameExpression(cond1,cond2,constFunctions)) + return true; + + // bitwise overlap for example 'x&7' and 'x==1' + if (cond1->str() == "&" && cond1->astOperand1() && cond2->astOperand2()) { + const Token *expr1 = cond1->astOperand1(); + const Token *num1 = cond1->astOperand2(); + if (!num1) // unary operator& + return false; + if (!num1->isNumber()) + std::swap(expr1,num1); + if (!num1->isNumber() || MathLib::isNegative(num1->str())) + return false; + + if (!Token::Match(cond2, "&|==") || !cond2->astOperand1() || !cond2->astOperand2()) + return false; + const Token *expr2 = cond2->astOperand1(); + const Token *num2 = cond2->astOperand2(); + if (!num2->isNumber()) + std::swap(expr2,num2); + if (!num2->isNumber() || MathLib::isNegative(num2->str())) + return false; + + if (!isSameExpression(expr1,expr2,constFunctions)) + return false; + + const MathLib::bigint value1 = MathLib::toLongNumber(num1->str()); + const MathLib::bigint value2 = MathLib::toLongNumber(num2->str()); + return ((value1 & value2) == value2); + } + return false; +} + + +void CheckCondition::multiCondition() +{ + if (!_settings->isEnabled("style")) + return; + + const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); + + for (std::list::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) { + if (i->type != Scope::eIf || !Token::simpleMatch(i->classDef, "if (")) + continue; + + const Token * const cond1 = i->classDef->next()->astOperand2(); + + const Token * tok2 = i->classDef->next(); + while (tok2) { + tok2 = tok2->link(); + if (!Token::simpleMatch(tok2, ") {")) + break; + tok2 = tok2->linkAt(1); + if (!Token::simpleMatch(tok2, "} else { if (")) + break; + tok2 = tok2->tokAt(4); + + if (isOverlappingCond(cond1, tok2->astOperand2(), _settings->library.functionpure)) + multiConditionError(tok2, cond1->linenr()); + } + } +} + +void CheckCondition::multiConditionError(const Token *tok, unsigned int line1) +{ + std::ostringstream errmsg; + errmsg << "Expression is always false because 'else if' condition matches previous condition at line " + << line1 << "."; + + reportError(tok, Severity::style, "multiCondition", errmsg.str()); +} + +//--------------------------------------------------------------------------- +// Detect oppositing inner and outer conditions +//--------------------------------------------------------------------------- + +static bool isOppositeCond(const Token * const cond1, const Token * const cond2, const std::set &constFunctions) +{ + if (!cond1 || !cond2) + return false; + + if (cond1->str() == "!") + return isSameExpression(cond1->astOperand1(), cond2, constFunctions); + + if (cond2->str() == "!") + return isSameExpression(cond1, cond2->astOperand1(), constFunctions); + + if (!cond1->isComparisonOp() || !cond2->isComparisonOp()) + return false; + + const std::string &comp1 = cond1->str(); + + // condition found .. get comparator + std::string comp2; + if (isSameExpression(cond1->astOperand1(), cond2->astOperand1(), constFunctions) && + isSameExpression(cond1->astOperand2(), cond2->astOperand2(), constFunctions)) { + comp2 = cond2->str(); + } else if (isSameExpression(cond1->astOperand1(), cond2->astOperand2(), constFunctions) && + isSameExpression(cond1->astOperand2(), cond2->astOperand1(), constFunctions)) { + comp2 = cond2->str(); + if (comp2[0] == '>') + comp2[0] = '<'; + else if (comp2[0] == '<') + comp2[0] = '>'; + } + + // is condition opposite? + return ((comp1 == "==" && comp2 == "!=") || + (comp1 == "!=" && comp2 == "==") || + (comp1 == "<" && comp2 == ">=") || + (comp1 == "<" && comp2 == ">") || + (comp1 == "<=" && comp2 == ">") || + (comp1 == ">" && comp2 == "<=") || + (comp1 == ">" && comp2 == "<") || + (comp1 == ">=" && comp2 == "<")); +} + +void CheckCondition::oppositeInnerCondition() +{ + if (!_settings->isEnabled("warning")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + + for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { + if (scope->type != Scope::eIf) + continue; + + if (!Token::simpleMatch(scope->classDef->linkAt(1), ") {")) + continue; + + bool nonlocal = false; // nonlocal variable used in condition + std::set vars; // variables used in condition + for (const Token *cond = scope->classDef; cond != scope->classDef->linkAt(1); cond = cond->next()) { + if (cond->varId()) { + vars.insert(cond->varId()); + const Variable *var = cond->variable(); + nonlocal |= (var && (!var->isLocal() || var->isStatic()) && !var->isArgument()); + // TODO: if var is pointer check what it points at + nonlocal |= (var && (var->isPointer() || var->isReference())); + } else if (cond->isName()) { + // varid is 0. this is possibly a nonlocal variable.. + nonlocal |= (cond->astParent() && cond->astParent()->isConstOp()); + } + } + + // parse until inner condition is reached.. + const Token *ifToken = nullptr; + for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { + if (Token::simpleMatch(tok, "if (")) { + ifToken = tok; + break; + } + if (Token::Match(tok, "%type% (") && nonlocal) // function call -> bailout if there are nonlocal variables + break; + else if ((tok->varId() && vars.find(tok->varId()) != vars.end()) || + (!tok->varId() && nonlocal)) { + if (Token::Match(tok, "%var% ++|--|=")) + break; + if (Token::Match(tok, "%var% [")) { + const Token *tok2 = tok->linkAt(1); + while (Token::simpleMatch(tok2, "] [")) + tok2 = tok2->linkAt(1); + if (Token::simpleMatch(tok2, "] =")) + break; + } + if (Token::Match(tok->previous(), "++|--|& %var%")) + break; + if (Token::Match(tok->previous(), "[(,] %var% [,)]")) { + // is variable unchanged? default is false.. + bool unchanged = false; + + // locate start parentheses in function call.. + unsigned int argumentNumber = 0; + const Token *start = tok->previous(); + while (start && start->str() == ",") { + start = start->astParent(); + ++argumentNumber; + } + + start = start ? start->previous() : nullptr; + if (start && start->function()) { + const Variable *arg = start->function()->getArgumentVar(argumentNumber); + if (arg && !arg->isPointer() && !arg->isReference()) + unchanged = true; + } + + if (!unchanged) + break; + } + } + } + if (!ifToken) + continue; + + // Condition.. + const Token *cond1 = scope->classDef->next()->astOperand2(); + const Token *cond2 = ifToken->next()->astOperand2(); + + if (isOppositeCond(cond1, cond2, _settings->library.functionpure)) + oppositeInnerConditionError(scope->classDef, cond2); + } +} + +void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2) +{ + 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."); +} + +//--------------------------------------------------------------------------- +// if ((x != 1) || (x != 3)) // expression always true +// if ((x == 1) && (x == 3)) // expression always false +// if ((x < 1) && (x > 3)) // expression always false +// if ((x > 3) || (x < 10)) // expression always true +// if ((x > 5) && (x != 1)) // second comparison always true +// +// Check for suspect logic for an expression consisting of 2 comparison +// expressions with a shared variable and constants and a logical operator +// between them. +// +// Suggest a different logical operator when the logical operator between +// the comparisons is probably wrong. +// +// Inform that second comparison is always true when first comparison is true. +//--------------------------------------------------------------------------- + +static std::string invertOperatorForOperandSwap(std::string s) +{ + if (s[0] == '<') + s[0] = '>'; + else if (s[0] == '>') + s[0] = '<'; + return s; +} + +static bool checkIntRelation(const std::string &op, const MathLib::bigint value1, const MathLib::bigint value2) +{ + return (op == "==" && value1 == value2) || + (op == "!=" && value1 != value2) || + (op == ">" && value1 > value2) || + (op == ">=" && value1 >= value2) || + (op == "<" && value1 < value2) || + (op == "<=" && value1 <= value2); +} + +static bool checkFloatRelation(const std::string &op, const double value1, const double value2) +{ + return (op == ">" && value1 > value2) || + (op == ">=" && value1 >= value2) || + (op == "<" && value1 < value2) || + (op == "<=" && value1 <= value2); +} + +template static T getvalue(const int test, const T value1, const T value2) +{ + // test: + // 1 => return value that is less than both value1 and value2 + // 2 => return value1 + // 3 => return value that is between value1 and value2 + // 4 => return value2 + // 5 => return value that is larger than both value1 and value2 + switch (test) { + case 1: { + T ret = std::min(value1, value2); + if ((ret - (T)1) < ret) + return ret - (T)1; + else if ((ret / (T)2) < ret) + return ret / (T)2; + else if ((ret * (T)2) < ret) + return ret * (T)2; + return ret; + } + case 2: + return value1; + case 3: + return (value1 + value2) / (T)2; + case 4: + return value2; + case 5: { + T ret = std::max(value1, value2); + if ((ret + (T)1) > ret) + return ret + (T)1; + else if ((ret / (T)2) > ret) + return ret / (T)2; + else if ((ret * (T)2) > ret) + return ret * (T)2; + return ret; + } + }; + return 0; +} + +void CheckCondition::checkIncorrectLogicOperator() +{ + bool style = _settings->isEnabled("style"); + bool warning = _settings->isEnabled("warning"); + if (!style && !warning) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t ii = 0; ii < functions; ++ii) { + const Scope * scope = symbolDatabase->functionScopes[ii]; + + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + // Opposite comparisons + if (Token::Match(tok, "%oror%|&&") && + tok->astOperand1() && + tok->astOperand2() && + (tok->astOperand1()->isName() || tok->astOperand2()->isName()) && + isOppositeCond(tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) { + + const bool alwaysTrue(tok->str() == "||"); + incorrectLogicOperatorError(tok, tok->expressionString(), alwaysTrue); + } + + else if (Token::Match(tok, "&&|%oror%")) { + // Comparison #1 (LHS) + const Token *comp1 = tok->astOperand1(); + if (comp1 && comp1->str() == tok->str()) + comp1 = comp1->astOperand2(); + + // Comparison #2 (RHS) + const Token *comp2 = tok->astOperand2(); + + if (!comp1 || !comp1->isComparisonOp() || !comp1->astOperand1() || !comp1->astOperand2()) + continue; + if (!comp2 || !comp2->isComparisonOp() || !comp2->astOperand1() || !comp2->astOperand2()) + continue; + + std::string op1, value1; + const Token *expr1; + if (comp1->astOperand1()->isLiteral()) { + op1 = invertOperatorForOperandSwap(comp1->str()); + value1 = comp1->astOperand1()->str(); + expr1 = comp1->astOperand2(); + } else if (comp1->astOperand2()->isLiteral()) { + op1 = comp1->str(); + value1 = comp1->astOperand2()->str(); + expr1 = comp1->astOperand1(); + } else { + continue; + } + + std::string op2, value2; + const Token *expr2; + if (comp2->astOperand1()->isLiteral()) { + op2 = invertOperatorForOperandSwap(comp2->str()); + value2 = comp2->astOperand1()->str(); + expr2 = comp2->astOperand2(); + } else if (comp2->astOperand2()->isLiteral()) { + op2 = comp2->str(); + value2 = comp2->astOperand2()->str(); + expr2 = comp2->astOperand1(); + } else { + continue; + } + + // Only float and int values are currently handled + if (!MathLib::isInt(value1) && !MathLib::isFloat(value1)) + continue; + if (!MathLib::isInt(value2) && !MathLib::isFloat(value2)) + continue; + + if (isSameExpression(comp1, comp2, _settings->library.functionpure)) + continue; // same expressions => only report that there are same expressions + if (!isSameExpression(expr1, expr2, _settings->library.functionpure)) + continue; + + const bool isfloat = astIsFloat(expr1, true) || MathLib::isFloat(value1) || astIsFloat(expr2, true) || MathLib::isFloat(value2); + + // don't check floating point equality comparisons. that is bad + // and deserves different warnings. + if (isfloat && (op1 == "==" || op1 == "!=" || op2 == "==" || op2 == "!=")) + continue; + + // evaluate if expression is always true/false + bool alwaysTrue = true, alwaysFalse = true; + bool firstTrue = true, secondTrue = true; + for (int test = 1; test <= 5; ++test) { + // test: + // 1 => testvalue is less than both value1 and value2 + // 2 => testvalue is value1 + // 3 => testvalue is between value1 and value2 + // 4 => testvalue value2 + // 5 => testvalue is larger than both value1 and value2 + bool result1, result2; + if (isfloat) { + const double d1 = MathLib::toDoubleNumber(value1); + const double d2 = MathLib::toDoubleNumber(value2); + const double testvalue = getvalue(test, d1, d2); + result1 = checkFloatRelation(op1, testvalue, d1); + result2 = checkFloatRelation(op2, testvalue, d2); + } else { + const MathLib::bigint i1 = MathLib::toLongNumber(value1); + const MathLib::bigint i2 = MathLib::toLongNumber(value2); + const MathLib::bigint testvalue = getvalue(test, i1, i2); + result1 = checkIntRelation(op1, testvalue, i1); + result2 = checkIntRelation(op2, testvalue, i2); + } + if (tok->str() == "&&") { + alwaysTrue &= (result1 && result2); + alwaysFalse &= !(result1 && result2); + } else { + alwaysTrue &= (result1 || result2); + alwaysFalse &= !(result1 || result2); + } + firstTrue &= !(!result1 && result2); + secondTrue &= !(result1 && !result2); + } + + const std::string cond1str = (expr1->isName() ? expr1->str() : "EXPR") + " " + op1 + " " + value1; + const std::string cond2str = (expr2->isName() ? expr2->str() : "EXPR") + " " + op2 + " " + value2; + if (warning && (alwaysTrue || alwaysFalse)) { + const std::string text = cond1str + " " + tok->str() + " " + cond2str; + incorrectLogicOperatorError(tok, text, alwaysTrue); + } else if (style && secondTrue) { + const std::string text = "If " + cond1str + ", the comparison " + cond2str + + " is always " + (secondTrue ? "true" : "false") + "."; + redundantConditionError(tok, text); + } else if (style && firstTrue) { + //const std::string text = "The comparison " + cond1str + " is always " + + // (firstTrue ? "true" : "false") + " when " + + // cond2str + "."; + const std::string text = "If " + cond2str + ", the comparison " + cond1str + + " is always " + (firstTrue ? "true" : "false") + "."; + redundantConditionError(tok, text); + } + } + } + } +} + +void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always) +{ + if (always) + reportError(tok, Severity::warning, "incorrectLogicOperator", + "Logical disjunction always evaluates to true: " + condition + ".\n" + "Logical disjunction always evaluates to true: " + condition + ". " + "Are these conditions necessary? Did you intend to use && instead? Are the numbers correct? Are you comparing the correct variables?"); + else + reportError(tok, Severity::warning, "incorrectLogicOperator", + "Logical conjunction always evaluates to false: " + condition + ".\n" + "Logical conjunction always evaluates to false: " + condition + ". " + "Are these conditions necessary? Did you intend to use || instead? Are the numbers correct? Are you comparing the correct variables?"); +} + +void CheckCondition::redundantConditionError(const Token *tok, const std::string &text) +{ + reportError(tok, Severity::style, "redundantCondition", "Redundant condition: " + text); +} + +//----------------------------------------------------------------------------- +// Detect "(var % val1) > val2" where val2 is >= val1. +//----------------------------------------------------------------------------- +void CheckCondition::checkModuloAlwaysTrueFalse() +{ + if (!_settings->isEnabled("warning")) + 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 (!tok->isComparisonOp()) + continue; + const Token *num, *modulo; + if (Token::simpleMatch(tok->astOperand1(), "%") && Token::Match(tok->astOperand2(), "%num%")) { + modulo = tok->astOperand1(); + num = tok->astOperand2(); + } else if (Token::Match(tok->astOperand1(), "%num%") && Token::simpleMatch(tok->astOperand2(), "%")) { + num = tok->astOperand1(); + modulo = tok->astOperand2(); + } else { + continue; + } + + if (Token::Match(modulo->astOperand2(), "%num%") && + MathLib::isLessEqual(modulo->astOperand2()->str(), num->str())) + moduloAlwaysTrueFalseError(tok, modulo->astOperand2()->str()); + } + } +} + +void CheckCondition::moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal) +{ + reportError(tok, Severity::warning, "moduloAlwaysTrueFalse", + "Comparison of modulo result is predetermined, because it is always less than " + maxVal + "."); +} + +//--------------------------------------------------------------------------- +// Clarify condition '(x = a < 0)' into '((x = a) < 0)' or '(x = (a < 0))' +// Clarify condition '(a & b == c)' into '((a & b) == c)' or '(a & (b == c))' +//--------------------------------------------------------------------------- +void CheckCondition::clarifyCondition() +{ + if (!_settings->isEnabled("style")) + return; + + const bool isC = _tokenizer->isC(); + + 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, "( %var% [=&|^]")) { + for (const Token *tok2 = tok->tokAt(3); tok2; tok2 = tok2->next()) { + if (tok2->str() == "(" || tok2->str() == "[") + tok2 = tok2->link(); + else if (tok2->type() == Token::eComparisonOp) { + // This might be a template + if (!isC && tok2->link()) + break; + + clarifyConditionError(tok, tok->strAt(2) == "=", false); + break; + } else if (!tok2->isName() && !tok2->isNumber() && tok2->str() != ".") + break; + } + } + } + } + + // using boolean result in bitwise operation ! x [&|^] + 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%|!")) { + if (tok->link()) // don't write false positives when templates are used + continue; + + const Token *tok2 = tok->next(); + + // Todo: There are false positives if '(' if encountered. It + // is assumed there is something like '(char *)&..' and therefore + // it bails out. + if (Token::Match(tok2, "(|&")) + continue; + + while (tok2 && (tok2->isName() || tok2->isNumber() || Token::Match(tok2, ".|(|["))) { + if (Token::Match(tok2, "(|[")) + tok2 = tok2->link(); + tok2 = tok2->next(); + } + + if (Token::Match(tok2, "[&|^]")) { + // don't write false positives when templates are used + if (Token::Match(tok2, "&|* ,|>") || Token::simpleMatch(tok2->previous(), "const &")) + continue; + + // #3609 - CWinTraits::.. + if (!isC && Token::Match(tok->previous(), "%var% <")) { + const Token *tok3 = tok2; + while (Token::Match(tok3, "[&|^] %var%")) + tok3 = tok3->tokAt(2); + if (Token::Match(tok3, ",|>")) + continue; + } + + clarifyConditionError(tok, false, true); + } + } + } + } +} + +void CheckCondition::clarifyConditionError(const Token *tok, bool assign, bool boolop) +{ + std::string errmsg; + + if (assign) + errmsg = "Suspicious condition (assignment + comparison); Clarify expression with parentheses."; + + else if (boolop) + errmsg = "Boolean result is used in bitwise operation. Clarify expression with parentheses.\n" + "Suspicious expression. Boolean result is used in bitwise operation. The operator '!' " + "and the comparison operators have higher precedence than bitwise operators. " + "It is recommended that the expression is clarified with parentheses."; + else + errmsg = "Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n" + "Suspicious condition. Comparison operators have higher precedence than bitwise operators. " + "Please clarify the condition with parentheses."; + + reportError(tok, + Severity::style, + "clarifyCondition", + errmsg); +} diff --git a/lib/checkassignif.h b/lib/checkcondition.h similarity index 57% rename from lib/checkassignif.h rename to lib/checkcondition.h index 0046bf87a..39f7f031f 100644 --- a/lib/checkassignif.h +++ b/lib/checkcondition.h @@ -18,8 +18,8 @@ //--------------------------------------------------------------------------- -#ifndef checkassignifH -#define checkassignifH +#ifndef checkconditionH +#define checkconditionH //--------------------------------------------------------------------------- #include "config.h" @@ -30,30 +30,34 @@ /// @{ /** - * @brief Check for assignment / condition mismatches + * @brief Check for condition mismatches */ -class CPPCHECKLIB CheckAssignIf : public Check { +class CPPCHECKLIB CheckCondition : public Check { public: /** This constructor is used when registering the CheckAssignIf */ - CheckAssignIf() : Check(myName()) { + CheckCondition() : Check(myName()) { } /** This constructor is used when running checks. */ - CheckAssignIf(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) + CheckCondition(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) : Check(myName(), tokenizer, settings, errorLogger) { } void runChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { - CheckAssignIf checkAssignIf(tokenizer, settings, errorLogger); - checkAssignIf.multiCondition(); + CheckCondition checkCondition(tokenizer, settings, errorLogger); + checkCondition.multiCondition(); + checkCondition.clarifyCondition(); // not simplified because ifAssign } /** @brief Run checks against the simplified token list */ void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { - CheckAssignIf checkAssignIf(tokenizer, settings, errorLogger); - checkAssignIf.assignIf(); - checkAssignIf.comparison(); + CheckCondition checkCondition(tokenizer, settings, errorLogger); + checkCondition.assignIf(); + checkCondition.comparison(); + checkCondition.oppositeInnerCondition(); + checkCondition.checkIncorrectLogicOperator(); + checkCondition.checkModuloAlwaysTrueFalse(); } /** mismatching assignment / comparison */ @@ -73,41 +77,69 @@ public: /** match 'if' and 'else if' conditions */ void multiCondition(); + /** To check the dead code in a program, which is inaccessible due to the counter-conditions check in nested-if statements **/ + void oppositeInnerCondition(); + + /** @brief %Check for testing for mutual exclusion over ||*/ + void checkIncorrectLogicOperator(); + + /** @brief %Check for suspicious usage of modulo (e.g. "if(var % 4 == 4)") */ + void checkModuloAlwaysTrueFalse(); + + /** @brief Suspicious condition (assignment+comparison) */ + void clarifyCondition(); + private: void assignIfError(const Token *tok1, const Token *tok2, const std::string &condition, bool result); - void mismatchingBitAndError(const Token *tok1, const MathLib::bigint num1, const Token *tok2, const MathLib::bigint num2); - void comparisonError(const Token *tok, const std::string &bitop, MathLib::bigint value1, const std::string &op, MathLib::bigint value2, bool result); - void multiConditionError(const Token *tok, unsigned int line1); + void oppositeInnerConditionError(const Token *tok1, const Token* tok2); + + void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always); + void redundantConditionError(const Token *tok, const std::string &text); + + void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal); + + void clarifyConditionError(const Token *tok, bool assign, bool boolop); + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { - CheckAssignIf c(0, settings, errorLogger); + CheckCondition c(0, settings, errorLogger); + c.assignIfError(0, 0, "", false); c.comparisonError(0, "&", 6, "==", 1, false); c.multiConditionError(0,1); - c.mismatchingBitAndError(0,0xf0, 0, 1); + c.mismatchingBitAndError(0, 0xf0, 0, 1); + c.oppositeInnerConditionError(0, 0); + c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true); + c.redundantConditionError(0, "If x > 11 the condition x > 10 is always true."); + c.moduloAlwaysTrueFalseError(0, "1"); + c.clarifyConditionError(0, true, false); } static std::string myName() { - return "AssignIf"; + return "Condition"; } std::string classInfo() const { - return "Match assignments and conditions:\n" + return "Match conditions with assignments and other conditions:\n" "* Mismatching assignment and comparison => comparison is always true/false\n" "* Mismatching lhs and rhs in comparison => comparison is always true/false\n" "* Detect matching 'if' and 'else if' conditions\n" - "* Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n"; + "* Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n" + "* 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"; } }; /// @} //--------------------------------------------------------------------------- -#endif // checkassignifH +#endif // checkconditionH diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 65207226a..79abbd177 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -32,7 +32,7 @@ namespace { CheckOther instance; } -static bool astIsFloat(const Token *tok, bool unknown) +bool astIsFloat(const Token *tok, bool unknown) { if (tok->astOperand2() && (tok->str() == "." || tok->str() == "::")) return astIsFloat(tok->astOperand2(), unknown); @@ -155,47 +155,6 @@ bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions) -{ - if (!cond1 || !cond2) - return false; - - if (cond1->str() == "!") - return isSameExpression(cond1->astOperand1(), cond2, constFunctions); - - if (cond2->str() == "!") - return isSameExpression(cond1, cond2->astOperand1(), constFunctions); - - if (!cond1->isComparisonOp() || !cond2->isComparisonOp()) - return false; - - const std::string &comp1 = cond1->str(); - - // condition found .. get comparator - std::string comp2; - if (isSameExpression(cond1->astOperand1(), cond2->astOperand1(), constFunctions) && - isSameExpression(cond1->astOperand2(), cond2->astOperand2(), constFunctions)) { - comp2 = cond2->str(); - } else if (isSameExpression(cond1->astOperand1(), cond2->astOperand2(), constFunctions) && - isSameExpression(cond1->astOperand2(), cond2->astOperand1(), constFunctions)) { - comp2 = cond2->str(); - if (comp2[0] == '>') - comp2[0] = '<'; - else if (comp2[0] == '<') - comp2[0] = '>'; - } - - // is condition opposite? - return ((comp1 == "==" && comp2 == "!=") || - (comp1 == "!=" && comp2 == "==") || - (comp1 == "<" && comp2 == ">=") || - (comp1 == "<" && comp2 == ">") || - (comp1 == "<=" && comp2 == ">") || - (comp1 == ">" && comp2 == "<=") || - (comp1 == ">" && comp2 == "<") || - (comp1 == ">=" && comp2 == "<")); -} - //---------------------------------------------------------------------------------- // The return value of fgetc(), getc(), ungetc(), getchar() etc. is an integer value. // If this return value is stored in a character variable and then compared @@ -323,106 +282,6 @@ void CheckOther::clarifyCalculationError(const Token *tok, const std::string &op "The code '" + calc + "' should be written as either '" + s1 + "' or '" + s2 + "'."); } -//--------------------------------------------------------------------------- -// Clarify condition '(x = a < 0)' into '((x = a) < 0)' or '(x = (a < 0))' -// Clarify condition '(a & b == c)' into '((a & b) == c)' or '(a & (b == c))' -//--------------------------------------------------------------------------- -void CheckOther::clarifyCondition() -{ - if (!_settings->isEnabled("style")) - return; - - const bool isC = _tokenizer->isC(); - - 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, "( %var% [=&|^]")) { - for (const Token *tok2 = tok->tokAt(3); tok2; tok2 = tok2->next()) { - if (tok2->str() == "(" || tok2->str() == "[") - tok2 = tok2->link(); - else if (tok2->type() == Token::eComparisonOp) { - // This might be a template - if (!isC && tok2->link()) - break; - - clarifyConditionError(tok, tok->strAt(2) == "=", false); - break; - } else if (!tok2->isName() && !tok2->isNumber() && tok2->str() != ".") - break; - } - } - } - } - - // using boolean result in bitwise operation ! x [&|^] - 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%|!")) { - if (tok->link()) // don't write false positives when templates are used - continue; - - const Token *tok2 = tok->next(); - - // Todo: There are false positives if '(' if encountered. It - // is assumed there is something like '(char *)&..' and therefore - // it bails out. - if (Token::Match(tok2, "(|&")) - continue; - - while (tok2 && (tok2->isName() || tok2->isNumber() || Token::Match(tok2,".|(|["))) { - if (Token::Match(tok2, "(|[")) - tok2 = tok2->link(); - tok2 = tok2->next(); - } - - if (Token::Match(tok2, "[&|^]")) { - // don't write false positives when templates are used - if (Token::Match(tok2, "&|* ,|>") || Token::simpleMatch(tok2->previous(), "const &")) - continue; - - // #3609 - CWinTraits::.. - if (!isC && Token::Match(tok->previous(), "%var% <")) { - const Token *tok3 = tok2; - while (Token::Match(tok3, "[&|^] %var%")) - tok3 = tok3->tokAt(2); - if (Token::Match(tok3, ",|>")) - continue; - } - - clarifyConditionError(tok,false,true); - } - } - } - } -} - -void CheckOther::clarifyConditionError(const Token *tok, bool assign, bool boolop) -{ - std::string errmsg; - - if (assign) - errmsg = "Suspicious condition (assignment + comparison); Clarify expression with parentheses."; - - else if (boolop) - errmsg = "Boolean result is used in bitwise operation. Clarify expression with parentheses.\n" - "Suspicious expression. Boolean result is used in bitwise operation. The operator '!' " - "and the comparison operators have higher precedence than bitwise operators. " - "It is recommended that the expression is clarified with parentheses."; - else - errmsg = "Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n" - "Suspicious condition. Comparison operators have higher precedence than bitwise operators. " - "Please clarify the condition with parentheses."; - - reportError(tok, - Severity::style, - "clarifyCondition", - errmsg); -} - //--------------------------------------------------------------------------- // Clarify (meaningless) statements like *foo++; with parentheses. //--------------------------------------------------------------------------- @@ -1178,249 +1037,6 @@ void CheckOther::suspiciousEqualityComparisonError(const Token* tok) } -//--------------------------------------------------------------------------- -// if ((x != 1) || (x != 3)) // expression always true -// if ((x == 1) && (x == 3)) // expression always false -// if ((x < 1) && (x > 3)) // expression always false -// if ((x > 3) || (x < 10)) // expression always true -// if ((x > 5) && (x != 1)) // second comparison always true -// -// Check for suspect logic for an expression consisting of 2 comparison -// expressions with a shared variable and constants and a logical operator -// between them. -// -// Suggest a different logical operator when the logical operator between -// the comparisons is probably wrong. -// -// Inform that second comparison is always true when first comparison is true. -//--------------------------------------------------------------------------- - -static std::string invertOperatorForOperandSwap(std::string s) -{ - if (s[0] == '<') - s[0] = '>'; - else if (s[0] == '>') - s[0] = '<'; - return s; -} - -static bool checkIntRelation(const std::string &op, const MathLib::bigint value1, const MathLib::bigint value2) -{ - return (op == "==" && value1 == value2) || - (op == "!=" && value1 != value2) || - (op == ">" && value1 > value2) || - (op == ">=" && value1 >= value2) || - (op == "<" && value1 < value2) || - (op == "<=" && value1 <= value2); -} - -static bool checkFloatRelation(const std::string &op, const double value1, const double value2) -{ - return (op == ">" && value1 > value2) || - (op == ">=" && value1 >= value2) || - (op == "<" && value1 < value2) || - (op == "<=" && value1 <= value2); -} - -template static T getvalue(const int test, const T value1, const T value2) -{ - // test: - // 1 => return value that is less than both value1 and value2 - // 2 => return value1 - // 3 => return value that is between value1 and value2 - // 4 => return value2 - // 5 => return value that is larger than both value1 and value2 - switch (test) { - case 1: { - T ret = std::min(value1, value2); - if ((ret - (T)1) < ret) - return ret - (T)1; - else if ((ret / (T)2) < ret) - return ret / (T)2; - else if ((ret * (T)2) < ret) - return ret * (T)2; - return ret; - } - case 2: - return value1; - case 3: - return (value1 + value2) / (T)2; - case 4: - return value2; - case 5: { - T ret = std::max(value1, value2); - if ((ret + (T)1) > ret) - return ret + (T)1; - else if ((ret / (T)2) > ret) - return ret / (T)2; - else if ((ret * (T)2) > ret) - return ret * (T)2; - return ret; - } - }; - return 0; -} - -void CheckOther::checkIncorrectLogicOperator() -{ - bool style = _settings->isEnabled("style"); - bool warning = _settings->isEnabled("warning"); - if (!style && !warning) - return; - - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); - const std::size_t functions = symbolDatabase->functionScopes.size(); - for (std::size_t ii = 0; ii < functions; ++ii) { - const Scope * scope = symbolDatabase->functionScopes[ii]; - - for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { - // Opposite comparisons - if (Token::Match(tok, "%oror%|&&") && - tok->astOperand1() && - tok->astOperand2() && - (tok->astOperand1()->isName() || tok->astOperand2()->isName()) && - isOppositeCond(tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) { - - const bool alwaysTrue(tok->str() == "||"); - incorrectLogicOperatorError(tok, tok->expressionString(), alwaysTrue); - } - - else if (Token::Match(tok, "&&|%oror%")) { - // Comparison #1 (LHS) - const Token *comp1 = tok->astOperand1(); - if (comp1 && comp1->str() == tok->str()) - comp1 = comp1->astOperand2(); - - // Comparison #2 (RHS) - const Token *comp2 = tok->astOperand2(); - - if (!comp1 || !comp1->isComparisonOp() || !comp1->astOperand1() || !comp1->astOperand2()) - continue; - if (!comp2 || !comp2->isComparisonOp() || !comp2->astOperand1() || !comp2->astOperand2()) - continue; - - std::string op1, value1; - const Token *expr1; - if (comp1->astOperand1()->isLiteral()) { - op1 = invertOperatorForOperandSwap(comp1->str()); - value1 = comp1->astOperand1()->str(); - expr1 = comp1->astOperand2(); - } else if (comp1->astOperand2()->isLiteral()) { - op1 = comp1->str(); - value1 = comp1->astOperand2()->str(); - expr1 = comp1->astOperand1(); - } else { - continue; - } - - std::string op2, value2; - const Token *expr2; - if (comp2->astOperand1()->isLiteral()) { - op2 = invertOperatorForOperandSwap(comp2->str()); - value2 = comp2->astOperand1()->str(); - expr2 = comp2->astOperand2(); - } else if (comp2->astOperand2()->isLiteral()) { - op2 = comp2->str(); - value2 = comp2->astOperand2()->str(); - expr2 = comp2->astOperand1(); - } else { - continue; - } - - // Only float and int values are currently handled - if (!MathLib::isInt(value1) && !MathLib::isFloat(value1)) - continue; - if (!MathLib::isInt(value2) && !MathLib::isFloat(value2)) - continue; - - if (isSameExpression(comp1, comp2, _settings->library.functionpure)) - continue; // same expressions => only report that there are same expressions - if (!isSameExpression(expr1, expr2, _settings->library.functionpure)) - continue; - - const bool isfloat = astIsFloat(expr1,true) || MathLib::isFloat(value1) || astIsFloat(expr2,true) || MathLib::isFloat(value2); - - // don't check floating point equality comparisons. that is bad - // and deserves different warnings. - if (isfloat && (op1=="==" || op1=="!=" || op2=="==" || op2=="!=")) - continue; - - // evaluate if expression is always true/false - bool alwaysTrue = true, alwaysFalse = true; - bool firstTrue = true, secondTrue = true; - for (int test = 1; test <= 5; ++test) { - // test: - // 1 => testvalue is less than both value1 and value2 - // 2 => testvalue is value1 - // 3 => testvalue is between value1 and value2 - // 4 => testvalue value2 - // 5 => testvalue is larger than both value1 and value2 - bool result1, result2; - if (isfloat) { - const double d1 = MathLib::toDoubleNumber(value1); - const double d2 = MathLib::toDoubleNumber(value2); - const double testvalue = getvalue(test, d1, d2); - result1 = checkFloatRelation(op1, testvalue, d1); - result2 = checkFloatRelation(op2, testvalue, d2); - } else { - const MathLib::bigint i1 = MathLib::toLongNumber(value1); - const MathLib::bigint i2 = MathLib::toLongNumber(value2); - const MathLib::bigint testvalue = getvalue(test, i1, i2); - result1 = checkIntRelation(op1, testvalue, i1); - result2 = checkIntRelation(op2, testvalue, i2); - } - if (tok->str() == "&&") { - alwaysTrue &= (result1 && result2); - alwaysFalse &= !(result1 && result2); - } else { - alwaysTrue &= (result1 || result2); - alwaysFalse &= !(result1 || result2); - } - firstTrue &= !(!result1 && result2); - secondTrue &= !(result1 && !result2); - } - - const std::string cond1str = (expr1->isName() ? expr1->str() : "EXPR") + " " + op1 + " " + value1; - const std::string cond2str = (expr2->isName() ? expr2->str() : "EXPR") + " " + op2 + " " + value2; - if (warning && (alwaysTrue || alwaysFalse)) { - const std::string text = cond1str + " " + tok->str() + " " + cond2str; - incorrectLogicOperatorError(tok, text, alwaysTrue); - } else if (style && secondTrue) { - const std::string text = "If " + cond1str + ", the comparison " + cond2str + - " is always " + (secondTrue ? "true" : "false") + "."; - redundantConditionError(tok, text); - } else if (style && firstTrue) { - //const std::string text = "The comparison " + cond1str + " is always " + - // (firstTrue ? "true" : "false") + " when " + - // cond2str + "."; - const std::string text = "If " + cond2str + ", the comparison " + cond1str + - " is always " + (firstTrue ? "true" : "false") + "."; - redundantConditionError(tok, text); - } - } - } - } -} - -void CheckOther::incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always) -{ - if (always) - reportError(tok, Severity::warning, "incorrectLogicOperator", - "Logical disjunction always evaluates to true: " + condition + ".\n" - "Logical disjunction always evaluates to true: " + condition + ". " - "Are these conditions necessary? Did you intend to use && instead? Are the numbers correct? Are you comparing the correct variables?"); - else - reportError(tok, Severity::warning, "incorrectLogicOperator", - "Logical conjunction always evaluates to false: " + condition + ".\n" - "Logical conjunction always evaluates to false: " + condition + ". " - "Are these conditions necessary? Did you intend to use || instead? Are the numbers correct? Are you comparing the correct variables?"); -} - -void CheckOther::redundantConditionError(const Token *tok, const std::string &text) -{ - reportError(tok, Severity::style, "redundantCondition", "Redundant condition: " + text); -} - //--------------------------------------------------------------------------- // strtol(str, 0, radix) <- radix must be 0 or 2-36 //--------------------------------------------------------------------------- @@ -2772,45 +2388,6 @@ void CheckOther::checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* to "for both parameters leads to a statement which is always " + strResult + "."); } -//----------------------------------------------------------------------------- -// Detect "(var % val1) > val2" where val2 is >= val1. -//----------------------------------------------------------------------------- -void CheckOther::checkModuloAlwaysTrueFalse() -{ - if (!_settings->isEnabled("warning")) - 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 (!tok->isComparisonOp()) - continue; - const Token *num,*modulo; - if (Token::simpleMatch(tok->astOperand1(), "%") && Token::Match(tok->astOperand2(), "%num%")) { - modulo = tok->astOperand1(); - num = tok->astOperand2(); - } else if (Token::Match(tok->astOperand1(), "%num%") && Token::simpleMatch(tok->astOperand2(), "%")) { - num = tok->astOperand1(); - modulo = tok->astOperand2(); - } else { - continue; - } - - if (Token::Match(modulo->astOperand2(), "%num%") && - MathLib::isLessEqual(modulo->astOperand2()->str(),num->str())) - moduloAlwaysTrueFalseError(tok, modulo->astOperand2()->str()); - } - } -} - -void CheckOther::moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal) -{ - reportError(tok, Severity::warning, "moduloAlwaysTrueFalse", - "Comparison of modulo result is predetermined, because it is always less than " + maxVal + "."); -} - //----------------------------------------------------------------------------- // Check for code like: // seteuid(geteuid()) or setuid(getuid()), which first gets and then sets the @@ -3097,107 +2674,6 @@ void CheckOther::incompleteArrayFillError(const Token* tok, const std::string& b "The array '" + buffer + "' is filled incompletely. The function '" + function + "()' needs the size given in bytes, but an element of the given array is larger than one byte. Did you forget to multiply the size with 'sizeof(*" + buffer + ")'?", true); } - -//--------------------------------------------------------------------------- -// Detect oppositing inner and outer conditions -//--------------------------------------------------------------------------- - -void CheckOther::oppositeInnerCondition() -{ - if (!_settings->isEnabled("warning")) - return; - - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); - - for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - if (scope->type != Scope::eIf) - continue; - - if (!Token::simpleMatch(scope->classDef->linkAt(1), ") {")) - continue; - - bool nonlocal = false; // nonlocal variable used in condition - std::set vars; // variables used in condition - for (const Token *cond = scope->classDef; cond != scope->classDef->linkAt(1); cond = cond->next()) { - if (cond->varId()) { - vars.insert(cond->varId()); - const Variable *var = cond->variable(); - nonlocal |= (var && (!var->isLocal() || var->isStatic()) && !var->isArgument()); - // TODO: if var is pointer check what it points at - nonlocal |= (var && (var->isPointer() || var->isReference())); - } else if (cond->isName()) { - // varid is 0. this is possibly a nonlocal variable.. - nonlocal |= (cond->astParent() && cond->astParent()->isConstOp()); - } - } - - // parse until inner condition is reached.. - const Token *ifToken = nullptr; - for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { - if (Token::simpleMatch(tok, "if (")) { - ifToken = tok; - break; - } - if (Token::Match(tok,"%type% (") && nonlocal) // function call -> bailout if there are nonlocal variables - break; - else if ((tok->varId() && vars.find(tok->varId()) != vars.end()) || - (!tok->varId() && nonlocal)) { - if (Token::Match(tok, "%var% ++|--|=")) - break; - if (Token::Match(tok,"%var% [")) { - const Token *tok2 = tok->linkAt(1); - while (Token::simpleMatch(tok2, "] [")) - tok2 = tok2->linkAt(1); - if (Token::simpleMatch(tok2, "] =")) - break; - } - if (Token::Match(tok->previous(), "++|--|& %var%")) - break; - if (Token::Match(tok->previous(), "[(,] %var% [,)]")) { - // is variable unchanged? default is false.. - bool unchanged = false; - - // locate start parentheses in function call.. - unsigned int argumentNumber = 0; - const Token *start = tok->previous(); - while (start && start->str() == ",") { - start = start->astParent(); - ++argumentNumber; - } - - start = start ? start->previous() : nullptr; - if (start && start->function()) { - const Variable *arg = start->function()->getArgumentVar(argumentNumber); - if (arg && !arg->isPointer() && !arg->isReference()) - unchanged = true; - } - - if (!unchanged) - break; - } - } - } - if (!ifToken) - continue; - - // Condition.. - const Token *cond1 = scope->classDef->next()->astOperand2(); - const Token *cond2 = ifToken->next()->astOperand2(); - - if (isOppositeCond(cond1, cond2, _settings->library.functionpure)) - oppositeInnerConditionError(scope->classDef, cond2); - } -} - -void CheckOther::oppositeInnerConditionError(const Token *tok1, const Token* tok2) -{ - 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."); -} - - //--------------------------------------------------------------------------- // Detect NULL being passed to variadic function. //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 15f0af45c..12ea6de43 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -31,6 +31,9 @@ class Variable; /** Is expressions same? */ bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions); +/** Is expression of floating point type? */ +bool astIsFloat(const Token *tok, bool unknown); + /// @addtogroup Checks /// @{ @@ -66,7 +69,6 @@ public: checkOther.checkUnreachableCode(); checkOther.checkSuspiciousSemicolon(); checkOther.checkVariableScope(); - checkOther.clarifyCondition(); // not simplified because ifAssign checkOther.checkSignOfUnsignedVariable(); // don't ignore casts (#3574) checkOther.checkIncompleteArrayFill(); checkOther.checkVarFuncNullUB(); @@ -79,7 +81,6 @@ public: CheckOther checkOther(tokenizer, settings, errorLogger); // Checks - checkOther.oppositeInnerCondition(); checkOther.clarifyCalculation(); checkOther.clarifyStatement(); checkOther.checkConstantFunctionParameter(); @@ -91,12 +92,10 @@ public: checkOther.checkMathFunctions(); checkOther.redundantGetAndSetUserId(); - checkOther.checkIncorrectLogicOperator(); checkOther.checkMisusedScopedObject(); checkOther.checkMemsetZeroBytes(); checkOther.checkMemsetInvalid2ndParam(); checkOther.checkSwitchCaseFallThrough(); - checkOther.checkModuloAlwaysTrueFalse(); checkOther.checkPipeParameterSize(); checkOther.checkInvalidFree(); @@ -107,15 +106,9 @@ public: checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse(); } - /** To check the dead code in a program, which is inaccessible due to the counter-conditions check in nested-if statements **/ - void oppositeInnerCondition(); - /** @brief Clarify calculation for ".. a * b ? .." */ void clarifyCalculation(); - /** @brief Suspicious condition (assignment+comparison) */ - void clarifyCondition(); - /** @brief Suspicious statement like '*A++;' */ void clarifyStatement(); @@ -184,9 +177,6 @@ public: /** @brief %Check for switch case fall through without comment */ void checkSwitchCaseFallThrough(); - /** @brief %Check for testing for mutual exclusion over ||*/ - void checkIncorrectLogicOperator(); - /** @brief %Check for objects that are destroyed immediately */ void checkMisusedScopedObject(); @@ -205,9 +195,6 @@ public: /** @brief %Check for suspicious code with the same expression on both sides of operator (e.g "if (a && a)") */ void checkDuplicateExpression(); - /** @brief %Check for suspicious usage of modulo (e.g. "if(var % 4 == 4)") */ - void checkModuloAlwaysTrueFalse(); - /** @brief %Check for code that gets never executed, such as duplicate break statements */ void checkUnreachableCode(); @@ -254,9 +241,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 *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); void redundantGetAndSetUserIdError(const Token *tok); void cstyleCastError(const Token *tok); @@ -282,8 +267,6 @@ private: void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString); void suspiciousEqualityComparisonError(const Token* tok); void selfAssignmentError(const Token *tok, const std::string &varname); - void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always); - void redundantConditionError(const Token *tok, const std::string &text); void misusedScopeObjectError(const Token *tok, const std::string &varname); void memsetZeroBytesError(const Token *tok, const std::string &varname); void memsetFloatError(const Token *tok, const std::string &var_value); @@ -301,7 +284,6 @@ private: void pointerPositiveError(const Token *tok, bool inconclusive); void SuspiciousSemicolonError(const Token *tok); void doubleCloseDirError(const Token *tok, const std::string &varname); - void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal); void negativeBitwiseShiftError(const Token *tok); void redundantCopyError(const Token *tok, const std::string &varname); void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean); @@ -332,7 +314,6 @@ private: // style/warning c.checkComparisonFunctionIsAlwaysTrueOrFalseError(0,"isless","varName",false); c.checkCastIntToCharAndBackError(0,"func_name"); - c.oppositeInnerConditionError(0, 0); c.cstyleCastError(0); c.passedByValueError(0, "parametername"); c.constStatementError(0, "type"); @@ -345,13 +326,10 @@ private: c.suspiciousCaseInSwitchError(0, "||"); c.suspiciousEqualityComparisonError(0); c.selfAssignmentError(0, "varname"); - c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true); - c.redundantConditionError(0, "If x > 11 the condition x > 10 is always true."); c.memsetZeroBytesError(0, "varname"); c.memsetFloatError(0, "varname"); c.memsetValueOutOfRangeError(0, "varname"); c.clarifyCalculationError(0, "+"); - c.clarifyConditionError(0, true, false); c.clarifyStatementError(0); c.duplicateBranchError(0, 0); c.duplicateExpressionError(0, 0, "&&"); @@ -362,7 +340,6 @@ private: c.pointerLessThanZeroError(0, false); c.pointerPositiveError(0, false); c.SuspiciousSemicolonError(0); - c.moduloAlwaysTrueFalseError(0, "1"); c.incompleteArrayFillError(0, "buffer", "memset", false); c.varFuncNullUBError(0); c.nanInArithmeticExpressionError(0); @@ -400,7 +377,6 @@ private: "* memset() with a float as the 2nd parameter\n" // style - "* Find dead code which is inaccessible due to the counter-conditions check in nested if statements\n" "* C-style pointer cast in cpp file\n" "* casting between incompatible pointer types\n" "* redundant if\n" @@ -409,7 +385,6 @@ private: "* [[IncompleteStatement|Incomplete statement]]\n" "* [[charvar|check how signed char variables are used]]\n" "* variable scope can be limited\n" - "* condition that is always true/false\n" "* unusual pointer arithmetic. For example: \"abc\" + 'd'\n" "* redundant assignment in a switch statement\n" "* redundant pre/post operation in a switch statement\n" @@ -418,17 +393,14 @@ private: "* assignment of a variable to itself\n" "* Suspicious case labels in switch()\n" "* Suspicious equality comparisons\n" - "* mutual exclusion over || always evaluating to true\n" "* Comparison of values leading always to true or false\n" "* Clarify calculation with parentheses\n" - "* suspicious condition (assignment+comparison)\n" "* suspicious comparison of '\\0' with a char* variable\n" "* duplicate break statement\n" "* unreachable code\n" "* testing if unsigned variable is negative\n" "* testing is unsigned variable is positive\n" "* Suspicious use of ; at the end of 'if/for/while' statement.\n" - "* Comparisons of modulo results that are always true/false.\n" "* Array filled incompletely using memset/memcpy/memmove.\n" "* redundant get and set function of user id (--std=posix).\n" "* Passing NULL pointer to function with variable number of arguments leads to UB on some platforms.\n" diff --git a/lib/cppcheck.vcxproj b/lib/cppcheck.vcxproj index b6cd46491..880ece4c0 100644 --- a/lib/cppcheck.vcxproj +++ b/lib/cppcheck.vcxproj @@ -39,12 +39,12 @@ - + @@ -84,12 +84,12 @@ - + diff --git a/lib/cppcheck.vcxproj.filters b/lib/cppcheck.vcxproj.filters index 9fa74291f..710361524 100644 --- a/lib/cppcheck.vcxproj.filters +++ b/lib/cppcheck.vcxproj.filters @@ -95,9 +95,6 @@ Source Files - - Source Files - Source Files @@ -140,6 +137,9 @@ Source Files + + Source Files + @@ -226,9 +226,6 @@ Header Files - - Header Files - Header Files @@ -277,6 +274,9 @@ Header Files + + Header Files + diff --git a/test/testassignif.cpp b/test/testassignif.cpp deleted file mode 100644 index 5e6be9c0c..000000000 --- a/test/testassignif.cpp +++ /dev/null @@ -1,395 +0,0 @@ -/* - * Cppcheck - A tool for static C/C++ code analysis - * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - - -#include "tokenize.h" -#include "checkassignif.h" -#include "testsuite.h" -#include - -extern std::ostringstream errout; - -class TestAssignIf : public TestFixture { -public: - TestAssignIf() : TestFixture("TestAssignIf") { - } - -private: - - - void run() { - TEST_CASE(assignAndCompare); // assignment and comparison don't match - TEST_CASE(mismatchingBitAnd); // overlapping bitmasks - TEST_CASE(compare); // mismatching LHS/RHS in comparison - TEST_CASE(multicompare); // mismatching comparisons - TEST_CASE(duplicateIf); // duplicate conditions in if and else-if - TEST_CASE(invalidMissingSemicolon); // crash as of #5867 - } - - void check(const char code[], bool validate=true) { - // Clear the error buffer.. - errout.str(""); - - Settings settings; - settings.addEnabled("style"); - - CheckAssignIf checkAssignIf; - - // Tokenize.. - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - checkAssignIf.runChecks(&tokenizer, &settings, this); - const std::string str1(tokenizer.tokens()->stringifyList(0,true)); - tokenizer.simplifyTokenList2(); - const std::string str2(tokenizer.tokens()->stringifyList(0,true)); - checkAssignIf.runSimplifiedChecks(&tokenizer, &settings, this); - - // Ensure that the test case is not bad. - if (validate && str1 != str2) { - warn(("Unsimplified code in test case. It looks like this test " - "should either be cleaned up or moved to TestTokenizer or " - "TestSimplifyTokens instead.\nstr1="+str1+"\nstr2="+str2).c_str()); - } - } - - void assignAndCompare() { - // & - check("void foo(int x)\n" - "{\n" - " int y = x & 4;\n" - " if (y == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y==3' is always false.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " int y = x & 4;\n" - " if (y != 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y!=3' is always true.\n", errout.str()); - - // | - check("void foo(int x) {\n" - " int y = x | 0x14;\n" - " if (y == 0x710);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==1808' is always false.\n", errout.str()); - - check("void foo(int x) {\n" - " int y = x | 0x14;\n" - " if (y == 0x71f);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // various simple assignments - check("void foo(int x) {\n" - " int y = (x+1) | 1;\n" - " if (y == 2);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==2' is always false.\n", errout.str()); - - check("void foo() {\n" - " int y = 1 | x();\n" - " if (y == 2);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==2' is always false.\n", errout.str()); - - // multiple conditions - check("void foo(int x) {\n" - " int y = x & 4;\n" - " if ((y == 3) && (z == 1));\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==3' is always false.\n", errout.str()); - - check("void foo(int x) {\n" - " int y = x & 4;\n" - " if ((x==123) || ((y == 3) && (z == 1)));\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==3' is always false.\n", errout.str()); - - check("void f(int x) {\n" - " int y = x & 7;\n" - " if (setvalue(&y) && y != 8);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // recursive checking into scopes - check("void f(int x) {\n" - " int y = x & 7;\n" - " if (z) y=0;\n" - " else { if (y==8); }\n" // always false - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); - - // while - check("void f(int x) {\n" - " int y = x & 7;\n" - " while (y==8);\n" // local variable => always false - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); - - check("void f(int x) {\n" - " extern int y; y = x & 7;\n" - " while (y==8);\n" // non-local variable => no error - "}"); - ASSERT_EQUALS("", errout.str()); - - // calling function - check("void f(int x) {\n" - " int y = x & 7;\n" - " do_something();\n" - " if (y==8);\n" // local variable => always false - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); - - check("void f(int x) {\n" - " int y = x & 7;\n" - " do_something(&y);\n" // passing variable => no error - " if (y==8);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void do_something(int);\n" - "void f(int x) {\n" - " int y = x & 7;\n" - " do_something(y);\n" - " if (y==8);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); - - check("void f(int x) {\n" - " extern int y; y = x & 7;\n" - " do_something();\n" - " if (y==8);\n" // non-local variable => no error - "}"); - ASSERT_EQUALS("", errout.str()); - - // #4434 : false positive: ?: - check("void f(int x) {\n" - " x = x & 1;\n" - " x = x & 1 ? 1 : -1;\n" - " if(x != -1) { }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #4735 - check("void f() {\n" - " int x = *(char*)&0x12345678;\n" - " if (x==18) { }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // bailout: no variable info - check("void foo(int x) {\n" - " y = 2 | x;\n" // y not declared => no error - " if(y == 1) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // bailout: negative number - check("void foo(int x) {\n" - " int y = -2 | x;\n" // negative number => no error - " if (y==1) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // bailout: pass variable to function - check("void foo(int x) {\n" - " int y = 2 | x;\n" - " bar(&y);\n" // pass variable to function => no error - " if (y==1) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // no crash on unary operator& (#5643) - check("SdrObject* ApplyGraphicToObject() {\n" - " if (&rHitObject) {}\n" - " else if (rHitObject.IsClosedObj() && !&rHitObject) { }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #5695: increment - check("void f(int a0, int n) {\n" - " int c = a0 & 3;\n" - " for (int a = 0; a < n; a++) {\n" - " c++;\n" - " if (c == 4)\n" - " c = 0;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } - - void mismatchingBitAnd() { - check("void f(int a) {\n" - " int b = a & 0xf0;\n" - " b &= 1;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching bitmasks. Result is always 0 (X = Y & 0xf0; Z = X & 0x1; => Z=0).\n", errout.str()); - - check("void f(int a) {\n" - " int b = a & 0xf0;\n" - " int c = b & 1;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching bitmasks. Result is always 0 (X = Y & 0xf0; Z = X & 0x1; => Z=0).\n", errout.str()); - - check("void f(int a) {\n" - " int b = a;" - " switch (x) {\n" - " case 1: b &= 1; break;\n" - " case 2: b &= 2; break;\n" - " };\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void compare() { - check("void foo(int x)\n" - "{\n" - " if ((x & 4) == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if ((x | 4) == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) == 0x3' is always false.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if ((x | 4) != 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) != 0x3' is always true.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if ((x & y & 4 & z ) == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); - } - - void multicompare() { - check("void foo(int x)\n" - "{\n" - " if (x & 7);\n" - " else { if (x == 1); }\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if (x & 7);\n" - " else { if (x & 1); }\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); - } - - void duplicateIf() { - check("void f(int a, int &b) {\n" - " if (a) { b = 1; }\n" - " else { if (a) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); - - check("void f(int a, int &b) {\n" - " if (a) { b = 1; }\n" - " else { if (a) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); - - check("void f(int a, int &b) {\n" - " if (a == 1) { b = 1; }\n" - " else { if (a == 2) { b = 2; }\n" - " else { if (a == 1) { b = 3; } } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); - - check("void f(int a, int &b) {\n" - " if (a == 1) { b = 1; }\n" - " else { if (a == 2) { b = 2; }\n" - " else { if (a == 2) { b = 3; } } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); - - check("void f(int a, int &b) {\n" - " if (a++) { b = 1; }\n" - " else { if (a++) { b = 2; }\n" - " else { if (a++) { b = 3; } } }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(int a, int &b) {\n" - " if (!strtok(NULL, \" \")) { b = 1; }\n" - " else { if (!strtok(NULL, \" \")) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(int a, int &b) {\n" - " x = x / 2;\n" - " if (x < 100) { b = 1; }\n" - " else { x = x / 2; if (x < 100) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(int i) {\n" - " if(i == 0x02e2000000 || i == 0xa0c6000000)\n" - " foo(i);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // ticket 3689 ( avoid false positive ) - check("int fitInt(long long int nValue){\n" - " if( nValue < 0x7fffffffLL )\n" - " {\n" - " return 32;\n" - " }\n" - " if( nValue < 0x7fffffffffffLL )\n" - " {\n" - " return 48;\n" - " }\n" - " else {\n" - " if( nValue < 0x7fffffffffffffffLL )\n" - " {\n" - " return 64;\n" - " } else\n" - " {\n" - " return -1;\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(WIDGET *widget) {\n" - " if (dynamic_cast(widget)){}\n" - " else if (dynamic_cast(widget)){}\n" - "}",false); - ASSERT_EQUALS("", errout.str()); - } - - void invalidMissingSemicolon() { - // simply survive - a syntax error would be even better - check("void f(int x) {\n" - " x = 42\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } -}; - -REGISTER_TEST(TestAssignIf) diff --git a/test/testcondition.cpp b/test/testcondition.cpp new file mode 100644 index 000000000..a30a77f1b --- /dev/null +++ b/test/testcondition.cpp @@ -0,0 +1,1188 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + + +#include "tokenize.h" +#include "checkcondition.h" +#include "testsuite.h" +#include + +extern std::ostringstream errout; + +class TestCondition : public TestFixture { +public: + TestCondition() : TestFixture("TestCondition") { + } + +private: + + + void run() { + TEST_CASE(assignAndCompare); // assignment and comparison don't match + TEST_CASE(mismatchingBitAnd); // overlapping bitmasks + TEST_CASE(compare); // mismatching LHS/RHS in comparison + TEST_CASE(multicompare); // mismatching comparisons + TEST_CASE(duplicateIf); // duplicate conditions in if and else-if + TEST_CASE(invalidMissingSemicolon); // crash as of #5867 + + TEST_CASE(incorrectLogicOperator1); + TEST_CASE(incorrectLogicOperator2); + TEST_CASE(incorrectLogicOperator3); + TEST_CASE(incorrectLogicOperator4); + TEST_CASE(incorrectLogicOperator5); // complex expressions + TEST_CASE(incorrectLogicOperator6); // char literals + TEST_CASE(incorrectLogicOperator7); // opposite expressions: (expr || !expr) + TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError); + TEST_CASE(incorrectLogicOp_condSwapping); + + TEST_CASE(modulo); + + TEST_CASE(oppositeInnerCondition); + + TEST_CASE(clarifyCondition1); // if (a = b() < 0) + TEST_CASE(clarifyCondition2); // if (a & b == c) + TEST_CASE(clarifyCondition3); // if (! a & b) + TEST_CASE(clarifyCondition4); // ticket #3110 + TEST_CASE(clarifyCondition5); // #3609 CWinTraits.. + TEST_CASE(clarifyCondition6); // #3818 + } + + void check(const char code[], bool validate=true, const char* filename = "test.cpp") { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("style"); + settings.addEnabled("warning"); + + CheckCondition checkCondition; + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, filename); + checkCondition.runChecks(&tokenizer, &settings, this); + const std::string str1(tokenizer.tokens()->stringifyList(0,true)); + tokenizer.simplifyTokenList2(); + const std::string str2(tokenizer.tokens()->stringifyList(0,true)); + checkCondition.runSimplifiedChecks(&tokenizer, &settings, this); + + // Ensure that the test case is not bad. + if (validate && str1 != str2) { + warn(("Unsimplified code in test case. It looks like this test " + "should either be cleaned up or moved to TestTokenizer or " + "TestSimplifyTokens instead.\nstr1="+str1+"\nstr2="+str2).c_str()); + } + } + + void assignAndCompare() { + // & + check("void foo(int x)\n" + "{\n" + " int y = x & 4;\n" + " if (y == 3);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y==3' is always false.\n", errout.str()); + + check("void foo(int x)\n" + "{\n" + " int y = x & 4;\n" + " if (y != 3);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y!=3' is always true.\n", errout.str()); + + // | + check("void foo(int x) {\n" + " int y = x | 0x14;\n" + " if (y == 0x710);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==1808' is always false.\n", errout.str()); + + check("void foo(int x) {\n" + " int y = x | 0x14;\n" + " if (y == 0x71f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // various simple assignments + check("void foo(int x) {\n" + " int y = (x+1) | 1;\n" + " if (y == 2);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==2' is always false.\n", errout.str()); + + check("void foo() {\n" + " int y = 1 | x();\n" + " if (y == 2);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==2' is always false.\n", errout.str()); + + // multiple conditions + check("void foo(int x) {\n" + " int y = x & 4;\n" + " if ((y == 3) && (z == 1));\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==3' is always false.\n", errout.str()); + + check("void foo(int x) {\n" + " int y = x & 4;\n" + " if ((x==123) || ((y == 3) && (z == 1)));\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==3' is always false.\n", errout.str()); + + check("void f(int x) {\n" + " int y = x & 7;\n" + " if (setvalue(&y) && y != 8);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // recursive checking into scopes + check("void f(int x) {\n" + " int y = x & 7;\n" + " if (z) y=0;\n" + " else { if (y==8); }\n" // always false + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); + + // while + check("void f(int x) {\n" + " int y = x & 7;\n" + " while (y==8);\n" // local variable => always false + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); + + check("void f(int x) {\n" + " extern int y; y = x & 7;\n" + " while (y==8);\n" // non-local variable => no error + "}"); + ASSERT_EQUALS("", errout.str()); + + // calling function + check("void f(int x) {\n" + " int y = x & 7;\n" + " do_something();\n" + " if (y==8);\n" // local variable => always false + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); + + check("void f(int x) {\n" + " int y = x & 7;\n" + " do_something(&y);\n" // passing variable => no error + " if (y==8);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void do_something(int);\n" + "void f(int x) {\n" + " int y = x & 7;\n" + " do_something(y);\n" + " if (y==8);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (style) Mismatching assignment and comparison, comparison 'y==8' is always false.\n", errout.str()); + + check("void f(int x) {\n" + " extern int y; y = x & 7;\n" + " do_something();\n" + " if (y==8);\n" // non-local variable => no error + "}"); + ASSERT_EQUALS("", errout.str()); + + // #4434 : false positive: ?: + check("void f(int x) {\n" + " x = x & 1;\n" + " x = x & 1 ? 1 : -1;\n" + " if(x != -1) { }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // #4735 + check("void f() {\n" + " int x = *(char*)&0x12345678;\n" + " if (x==18) { }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // bailout: no variable info + check("void foo(int x) {\n" + " y = 2 | x;\n" // y not declared => no error + " if(y == 1) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // bailout: negative number + check("void foo(int x) {\n" + " int y = -2 | x;\n" // negative number => no error + " if (y==1) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // bailout: pass variable to function + check("void foo(int x) {\n" + " int y = 2 | x;\n" + " bar(&y);\n" // pass variable to function => no error + " if (y==1) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // no crash on unary operator& (#5643) + check("SdrObject* ApplyGraphicToObject() {\n" + " if (&rHitObject) {}\n" + " else if (rHitObject.IsClosedObj() && !&rHitObject) { }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // #5695: increment + check("void f(int a0, int n) {\n" + " int c = a0 & 3;\n" + " for (int a = 0; a < n; a++) {\n" + " c++;\n" + " if (c == 4)\n" + " c = 0;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + + void mismatchingBitAnd() { + check("void f(int a) {\n" + " int b = a & 0xf0;\n" + " b &= 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching bitmasks. Result is always 0 (X = Y & 0xf0; Z = X & 0x1; => Z=0).\n", errout.str()); + + check("void f(int a) {\n" + " int b = a & 0xf0;\n" + " int c = b & 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Mismatching bitmasks. Result is always 0 (X = Y & 0xf0; Z = X & 0x1; => Z=0).\n", errout.str()); + + check("void f(int a) {\n" + " int b = a;" + " switch (x) {\n" + " case 1: b &= 1; break;\n" + " case 2: b &= 2; break;\n" + " };\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void compare() { + check("void foo(int x)\n" + "{\n" + " if ((x & 4) == 3);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); + + check("void foo(int x)\n" + "{\n" + " if ((x | 4) == 3);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) == 0x3' is always false.\n", errout.str()); + + check("void foo(int x)\n" + "{\n" + " if ((x | 4) != 3);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) != 0x3' is always true.\n", errout.str()); + + check("void foo(int x)\n" + "{\n" + " if ((x & y & 4 & z ) == 3);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); + } + + void multicompare() { + check("void foo(int x)\n" + "{\n" + " if (x & 7);\n" + " else { if (x == 1); }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); + + check("void foo(int x)\n" + "{\n" + " if (x & 7);\n" + " else { if (x & 1); }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); + } + + void duplicateIf() { + check("void f(int a, int &b) {\n" + " if (a) { b = 1; }\n" + " else { if (a) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a) { b = 1; }\n" + " else { if (a) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a == 1) { b = 1; }\n" + " else { if (a == 2) { b = 2; }\n" + " else { if (a == 1) { b = 3; } } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a == 1) { b = 1; }\n" + " else { if (a == 2) { b = 2; }\n" + " else { if (a == 2) { b = 3; } } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a++) { b = 1; }\n" + " else { if (a++) { b = 2; }\n" + " else { if (a++) { b = 3; } } }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int a, int &b) {\n" + " if (!strtok(NULL, \" \")) { b = 1; }\n" + " else { if (!strtok(NULL, \" \")) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int a, int &b) {\n" + " x = x / 2;\n" + " if (x < 100) { b = 1; }\n" + " else { x = x / 2; if (x < 100) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int i) {\n" + " if(i == 0x02e2000000 || i == 0xa0c6000000)\n" + " foo(i);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // ticket 3689 ( avoid false positive ) + check("int fitInt(long long int nValue){\n" + " if( nValue < 0x7fffffffLL )\n" + " {\n" + " return 32;\n" + " }\n" + " if( nValue < 0x7fffffffffffLL )\n" + " {\n" + " return 48;\n" + " }\n" + " else {\n" + " if( nValue < 0x7fffffffffffffffLL )\n" + " {\n" + " return 64;\n" + " } else\n" + " {\n" + " return -1;\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(WIDGET *widget) {\n" + " if (dynamic_cast(widget)){}\n" + " else if (dynamic_cast(widget)){}\n" + "}",false); + ASSERT_EQUALS("", errout.str()); + } + + void invalidMissingSemicolon() { + // simply survive - a syntax error would be even better + check("void f(int x) {\n" + " x = 42\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + + + void incorrectLogicOperator1() { + check("void f(int x) {\n" + " if ((x != 1) || (x != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (1 != x || 3 != x)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); + + check("void f(int x) {\n" // ast.. + " if (y == 1 && x == 1 && x == 7) { }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n", errout.str()); + + check("void f(int x, int y) {\n" + " if (x != 1 || y != 1)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x, int y) {\n" + " if ((y == 1) && (x != 1) || (x != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x, int y) {\n" + " if ((x != 1) || (x != 3) && (y == 1))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if ((x != 1) && (x != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if ((x == 1) || (x == 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x, int y) {\n" + " if ((x != 1) || (y != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x, int y) {\n" + " if ((x != hotdog) || (y != hotdog))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x, int y) {\n" + " if ((x != 5) || (y != 5))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + + check("void f(int x) {\n" + " if ((x != 5) || (x != 6))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n", errout.str()); + + check("void f(unsigned int a, unsigned int b, unsigned int c) {\n" + " if((a != b) || (c != b) || (c != a))\n" + " {\n" + " return true;\n" + " }\n" + " return false;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + } + + void incorrectLogicOperator2() { + check("void f(float x) {\n" + " if ((x == 1) && (x == 1.0))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if ((x == 1) && (x == 0x00000001))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x == 1 && x == 3)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (x == 1.0 && x == 3.0)\n" + " a++;\n" + "}"); + //ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1.0 && x == 3.0.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); // float comparisons with == and != are not checked right now - such comparison is a bad idea + + check("void f(float x) {\n" + " if (x == 1 && x == 1.0)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void bar(float f) {\n" // #5246 + " if ((f > 0) && (f < 1)) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x < 1 && x > 1)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (x < 1.0 && x > 1.0)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1.0 && x > 1.0.\n", errout.str()); + + check("void f(int x) {\n" + " if (x < 1 && x > 1.0)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 1.0.\n", errout.str()); + + check("void f(int x) {\n" + " if (x >= 1.0 && x <= 1.001)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x < 1 && x > 3)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); + + check("void f(float x) {\n" + " if (x < 1.0 && x > 3.0)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1.0 && x > 3.0.\n", errout.str()); + + check("void f(int x) {\n" + " if (1 > x && 3 < x)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (x < 3 && x > 1)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x > 3 || x < 10)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x > 3 || x < 10.\n", errout.str()); + + check("void f(int x) {\n" + " if (x >= 3 || x <= 10)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x <= 10.\n", errout.str()); + + check("void f(int x) {\n" + " if (x >= 3 || x < 10)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x < 10.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 3 || x <= 10)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x > 3 || x <= 10.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 3 || x < 3)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x >= 3 || x <= 3)\n" + " a++;\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x <= 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (x >= 3 || x < 3)\n" + " a++;\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x < 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 3 || x <= 3)\n" + " a++;\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x > 3 || x <= 3.\n", errout.str()); + + check("void f(int x) {\n" + " if((x==3) && (x!=4))\n" + " a++;\n" + "}"); + + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x!=4) && (x==3))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x==3) || (x!=4))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x!=4) || (x==3))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x==3) && (x!=3))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x==6) || (x!=6))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x == 6 || x != 6.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 10 || x < 3)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if (x > 5 && x == 1)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 5 && x == 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 5 && x == 6)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 6, the comparison x > 5 is always true.\n", errout.str()); + + // #3419 + check("void f() {\n" + " if ( &q != &a && &q != &b ) { }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // #3676 + check("void f(int m_x2, int w, int x) {\n" + " if (x + w - 1 > m_x2 || m_x2 < 0 )\n" + " m_x2 = x + w - 1;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(float x) {\n" // x+1 => x + " if (x <= 1.0e20 && x >= -1.0e20) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(float x) {\n" // x+1 => x + " if (x >= 1.0e20 && x <= 1.0e21) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(float x) {\n" // x+1 => x + " if (x <= -1.0e20 && x >= -1.0e21) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void incorrectLogicOperator3() { + check("void f(int x, bool& b) {\n" + " b = x > 5 && x == 1;\n" + " c = x < 1 && x == 3;\n" + " d = x >= 5 && x == 1;\n" + " e = x <= 1 && x == 3;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 5 && x == 1.\n" + "[test.cpp:3]: (warning) Logical conjunction always evaluates to false: x < 1 && x == 3.\n" + "[test.cpp:4]: (warning) Logical conjunction always evaluates to false: x >= 5 && x == 1.\n" + "[test.cpp:5]: (warning) Logical conjunction always evaluates to false: x <= 1 && x == 3.\n", errout.str()); + } + + void incorrectLogicOperator4() { + check("void f(int x) {\n" + " if (x && x != $0) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void incorrectLogicOperator5() { // complex expressions + check("void f(int x) {\n" + " if (x+3 > 2 || x+3 < 10) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: EXPR > 2 || EXPR < 10.\n", errout.str()); + } + + void incorrectLogicOperator6() { // char literals + check("void f(char x) {\n" + " if (x == '1' || x == '2') {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char x) {\n" + " if (x == '1' && x == '2') {}\n" + "}"); + TODO_ASSERT_EQUALS("error", "", errout.str()); + } + + void incorrectLogicOperator7() { // opposite expressions + check("void f(int i) {\n" + " if (i || !i) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: i||!i.\n", errout.str()); + } + + void secondAlwaysTrueFalseWhenFirstTrueError() { + check("void f(int x) {\n" + " if (x > 5 && x != 1)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 5 && x != 6)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if ((x > 5) && (x != 1))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x > 5) && (x != 6))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x, bool& b) {\n" + " b = x > 3 || x == 4;\n" + " c = x < 5 || x == 4;\n" + " d = x >= 3 || x == 4;\n" + " e = x <= 5 || x == 4;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 4, the comparison x > 3 is always true.\n" + "[test.cpp:3]: (style) Redundant condition: If x == 4, the comparison x < 5 is always true.\n" + "[test.cpp:4]: (style) Redundant condition: If x == 4, the comparison x >= 3 is always true.\n" + "[test.cpp:5]: (style) Redundant condition: If x == 4, the comparison x <= 5 is always true.\n", errout.str()); + + check("void f(int x, bool& b) {\n" + " b = x > 5 || x != 1;\n" + " c = x < 1 || x != 3;\n" + " d = x >= 5 || x != 1;\n" + " e = x <= 1 || x != 3;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n" + "[test.cpp:3]: (style) Redundant condition: If x < 1, the comparison x != 3 is always true.\n" + "[test.cpp:4]: (style) Redundant condition: If x >= 5, the comparison x != 1 is always true.\n" + "[test.cpp:5]: (style) Redundant condition: If x <= 1, the comparison x != 3 is always true.\n", errout.str()); + + check("void f(int x, bool& b) {\n" + " b = x > 6 && x > 5;\n" + " c = x > 5 || x > 6;\n" + " d = x < 6 && x < 5;\n" + " e = x < 5 || x < 6;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 6, the comparison x > 5 is always true.\n" + "[test.cpp:3]: (style) Redundant condition: If x > 6, the comparison x > 5 is always true.\n" + "[test.cpp:4]: (style) Redundant condition: If x < 5, the comparison x < 6 is always true.\n" + "[test.cpp:5]: (style) Redundant condition: If x < 5, the comparison x < 6 is always true.\n", errout.str()); + } + + void incorrectLogicOp_condSwapping() { + check("void f(int x) {\n" + " if (x < 1 && x > 3)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (1 > x && x > 3)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (x < 1 && 3 < x)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (1 > x && 3 < x)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 3 && x < 1)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (3 < x && x < 1)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (x > 3 && 1 > x)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); + + check("void f(int x) {\n" + " if (3 < x && 1 > x)\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); + } + + void modulo() { + check("bool f(bool& b1, bool& b2, bool& b3) {\n" + " b1 = a % 5 == 4;\n" + " b2 = a % c == 100000;\n" + " b3 = a % 5 == c;\n" + " return a % 5 == 5-p;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("bool f(bool& b1, bool& b2, bool& b3, bool& b4, bool& b5) {\n" + " b1 = a % 5 < 5;\n" + " b2 = a % 5 <= 5;\n" + " b3 = a % 5 == 5;\n" + " b4 = a % 5 != 5;\n" + " b5 = a % 5 >= 5;\n" + " return a % 5 > 5;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:5]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:6]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:7]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", errout.str()); + + check("void f(bool& b1, bool& b2) {\n" + " b1 = bar() % 5 < 889;\n" + " if(x[593] % 5 <= 5)\n" + " b2 = x.a % 5 == 5;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", errout.str()); + + check("void f() {\n" + " if (a % 2 + b % 2 == 2)\n" + " foo();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void oppositeInnerCondition() { + check("void foo(int a, int b) {\n" + " if(a==b)\n" + " if(a!=b)\n" + " cout << a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + + check("void foo(int a, int b) {\n" + " if(a==b)\n" + " if(b!=a)\n" + " cout << a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) 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) 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" + " if(i > 5) {\n" + " i = bar();\n" + " if(i < 5) {\n" + " cout << a;\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + + check("void foo(int& i) {\n" + " i=6;\n" + "}\n" + "void bar(int i) {\n" + " if(i>5) {\n" + " foo(i);\n" + " if(i<5) {\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(int& i);\n" + "void bar() {\n" + " int i; i = func();\n" + " if(i>5) {\n" + " foo(i);\n" + " if(i<5) {\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(int i);\n" + "void bar(int i) {\n" + " if(i>5) {\n" + " foo(i);\n" + " if(i<5) {\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + + check("void foo(int i);\n" + "void bar() {\n" + " int i; i = func();\n" + " if(i>5) {\n" + " foo(i);\n" + " if(i<5) {\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:6]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + + // see linux revision 1f80c0cc + check("int generic_write_sync(int,int,int);\n" + "\n" + "void cifs_writev(int i) {\n" + " int rc = __generic_file_aio_write();\n" + " if (rc > 0){\n" + " err = generic_write_sync(file, iocb->ki_pos - rc, rc);\n" + " if(rc < 0) {\n" // <- condition is always false + " err = rc;\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:7]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); + + check("void f(struct ABC *abc) {\n" + " struct AB *ab = abc->ab;\n" + " if (ab->a == 123){\n" + " do_something(abc);\n" // might change ab->a + " if (ab->a != 123) {\n" + " err = rc;\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // #5731 - fp when undeclared variable is used + check("void f() {\n" + " if (x == -1){\n" + " x = do_something();\n" + " if (x != -1) {}\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // #5750 - another fp when undeclared variable is used + check("void f() {\n" + " if (r < w){\n" + " r += 3;\n" + " if (r > w) {}\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // #5874 - array + check("void testOppositeConditions2() {\n" + " int array[2] = { 0, 0 };\n" + " if (array[0] < 2) {\n" + " array[0] += 5;\n" + " if (array[0] > 2) {}\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + // clarify conditions with = and comparison + void clarifyCondition1() { + check("void f() {\n" + " if (x = b() < 0) {}\n" // don't simplify and verify this code + "}", false); + ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.\n", errout.str()); + + check("void f(int i) {\n" + " for (i = 0; i < 10; i++) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " x = a(); if (x) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + } + + // clarify conditions with bitwise operator and comparison + void clarifyCondition2() { + check("void f() {\n" + " if (x & 3 == 2) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n", errout.str()); + + check("void f() {\n" + " if (a & fred1.x == fred2.y) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n", errout.str()); + } + + // clarify condition that uses ! operator and then bitwise operator + void clarifyCondition3() { + check("void f(int w) {\n" + " if(!w & 0x8000) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n", errout.str()); + + check("void f() {\n" + " if (x == foo() & 2) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n", errout.str()); + + check("void f(std::list &ints) { }"); + ASSERT_EQUALS("", errout.str()); + + check("void f() { A a; }"); + ASSERT_EQUALS("", errout.str()); + + check("void f() { a(x there are never templates + ASSERT_EQUALS("[test.c:1]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n", errout.str()); + + check("class A;", "test.C"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if (result != (char *)&inline_result) { }\n" // don't simplify and verify cast + "}", false); + ASSERT_EQUALS("", errout.str()); + } + + void clarifyCondition4() { // ticket #3110 + check("typedef double SomeType;\n" + "typedef std::pair PairType;\n" + "struct S\n" + "{\n" + " bool operator()\n" + " ( PairType const & left\n" + " , PairType const & right) const\n" + " {\n" + " return left.first < right.first;\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void clarifyCondition5() { // ticket #3609 (using | in template instantiation) + check("CWinTraits::GetWndStyle(0);"); + ASSERT_EQUALS("", errout.str()); + } + + void clarifyCondition6() { + check("template\n" + "SharedPtr& operator=( SharedPtr const & r ) {\n" + " px = r.px;\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } +}; + +REGISTER_TEST(TestCondition) diff --git a/test/testother.cpp b/test/testother.cpp index 34b9c330c..b7280bb65 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -36,7 +36,6 @@ private: void run() { - TEST_CASE(oppositeInnerCondition); TEST_CASE(emptyBrackets); TEST_CASE(zeroDiv1); @@ -117,17 +116,6 @@ private: TEST_CASE(trac2071); TEST_CASE(trac2084); TEST_CASE(trac3693); - TEST_CASE(modulo); - - TEST_CASE(incorrectLogicOperator1); - TEST_CASE(incorrectLogicOperator2); - TEST_CASE(incorrectLogicOperator3); - TEST_CASE(incorrectLogicOperator4); - TEST_CASE(incorrectLogicOperator5); // complex expressions - TEST_CASE(incorrectLogicOperator6); // char literals - TEST_CASE(incorrectLogicOperator7); // opposite expressions: (expr || !expr) - TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError); - TEST_CASE(incorrectLogicOp_condSwapping); TEST_CASE(memsetZeroBytes); TEST_CASE(memsetInvalid2ndParam); @@ -137,13 +125,6 @@ private: TEST_CASE(clarifyCalculation); TEST_CASE(clarifyStatement); - TEST_CASE(clarifyCondition1); // if (a = b() < 0) - TEST_CASE(clarifyCondition2); // if (a & b == c) - TEST_CASE(clarifyCondition3); // if (! a & b) - TEST_CASE(clarifyCondition4); // ticket #3110 - TEST_CASE(clarifyCondition5); // #3609 CWinTraits.. - TEST_CASE(clarifyCondition6); // #3818 - TEST_CASE(duplicateBranch); TEST_CASE(duplicateBranch1); // tests extracted by http://www.viva64.com/en/b/0149/ ( Comparison between PVS-Studio and cppcheck ): Errors detected in Quake 3: Arena by PVS-Studio: Fragement 2 TEST_CASE(duplicateBranch2); // empty macro @@ -267,171 +248,6 @@ private: } - void oppositeInnerCondition() { - check("void foo(int a, int b) {\n" - " if(a==b)\n" - " if(a!=b)\n" - " cout << a;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); - - check("void foo(int a, int b) {\n" - " if(a==b)\n" - " if(b!=a)\n" - " cout << a;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) 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) 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" - " if(i > 5) {\n" - " i = bar();\n" - " if(i < 5) {\n" - " cout << a;\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - - check("void foo(int& i) {\n" - " i=6;\n" - "}\n" - "void bar(int i) {\n" - " if(i>5) {\n" - " foo(i);\n" - " if(i<5) {\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void foo(int& i);\n" - "void bar() {\n" - " int i; i = func();\n" - " if(i>5) {\n" - " foo(i);\n" - " if(i<5) {\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void foo(int i);\n" - "void bar(int i) {\n" - " if(i>5) {\n" - " foo(i);\n" - " if(i<5) {\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); - - check("void foo(int i);\n" - "void bar() {\n" - " int i; i = func();\n" - " if(i>5) {\n" - " foo(i);\n" - " if(i<5) {\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:6]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); - - // see linux revision 1f80c0cc - check("int generic_write_sync(int,int,int);\n" - "\n" - "void cifs_writev(int i) {\n" - " int rc = __generic_file_aio_write();\n" - " if (rc > 0){\n" - " err = generic_write_sync(file, iocb->ki_pos - rc, rc);\n" - " if(rc < 0) {\n" // <- condition is always false - " err = rc;\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:7]: (warning) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str()); - - check("void f(struct ABC *abc) {\n" - " struct AB *ab = abc->ab;\n" - " if (ab->a == 123){\n" - " do_something(abc);\n" // might change ab->a - " if (ab->a != 123) {\n" - " err = rc;\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #5731 - fp when undeclared variable is used - check("void f() {\n" - " if (x == -1){\n" - " x = do_something();\n" - " if (x != -1) {}\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #5750 - another fp when undeclared variable is used - check("void f() {\n" - " if (r < w){\n" - " r += 3;\n" - " if (r > w) {}\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #5874 - array - check("void testOppositeConditions2() {\n" - " int array[2] = { 0, 0 };\n" - " if (array[0] < 2) {\n" - " array[0] += 5;\n" - " if (array[0] > 2) {}\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - void emptyBrackets() { check("{\n" "}"); @@ -3640,547 +3456,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void modulo() { - check("bool f(bool& b1, bool& b2, bool& b3) {\n" - " b1 = a % 5 == 4;\n" - " b2 = a % c == 100000;\n" - " b3 = a % 5 == c;\n" - " return a % 5 == 5-p;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("bool f(bool& b1, bool& b2, bool& b3, bool& b4, bool& b5) {\n" - " b1 = a % 5 < 5;\n" - " b2 = a % 5 <= 5;\n" - " b3 = a % 5 == 5;\n" - " b4 = a % 5 != 5;\n" - " b5 = a % 5 >= 5;\n" - " return a % 5 > 5;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:5]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:6]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:7]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", errout.str()); - - check("void f(bool& b1, bool& b2) {\n" - " b1 = bar() % 5 < 889;\n" - " if(x[593] % 5 <= 5)\n" - " b2 = x.a % 5 == 5;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", errout.str()); - - check("void f() {\n" - " if (a % 2 + b % 2 == 2)\n" - " foo();\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void incorrectLogicOperator1() { - check("void f(int x) {\n" - " if ((x != 1) || (x != 3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (1 != x || 3 != x)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); - - check("void f(int x) {\n" // ast.. - " if (y == 1 && x == 1 && x == 7) { }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n", errout.str()); - - check("void f(int x, int y) {\n" - " if (x != 1 || y != 1)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x, int y) {\n" - " if ((y == 1) && (x != 1) || (x != 3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x, int y) {\n" - " if ((x != 1) || (x != 3) && (y == 1))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if ((x != 1) && (x != 3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if ((x == 1) || (x == 3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x, int y) {\n" - " if ((x != 1) || (y != 3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x, int y) {\n" - " if ((x != hotdog) || (y != hotdog))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x, int y) {\n" - " if ((x != 5) || (y != 5))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - - check("void f(int x) {\n" - " if ((x != 5) || (x != 6))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n", errout.str()); - - check("void f(unsigned int a, unsigned int b, unsigned int c) {\n" - " if((a != b) || (c != b) || (c != a))\n" - " {\n" - " return true;\n" - " }\n" - " return false;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - } - - void incorrectLogicOperator2() { - check("void f(float x) {\n" - " if ((x == 1) && (x == 1.0))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if ((x == 1) && (x == 0x00000001))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&&'.\n", errout.str()); - - check("void f(int x) {\n" - " if (x == 1 && x == 3)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (x == 1.0 && x == 3.0)\n" - " a++;\n" - "}\n" - ); - //ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1.0 && x == 3.0.\n", errout.str()); - ASSERT_EQUALS("", errout.str()); // float comparisons with == and != are not checked right now - such comparison is a bad idea - - check("void f(float x) {\n" - " if (x == 1 && x == 1.0)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void bar(float f) {\n" // #5246 - " if ((f > 0) && (f < 1)) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if (x < 1 && x > 1)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 1.\n", errout.str()); - - check("void f(int x) {\n" - " if (x < 1.0 && x > 1.0)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1.0 && x > 1.0.\n", errout.str()); - - check("void f(int x) {\n" - " if (x < 1 && x > 1.0)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 1.0.\n", errout.str()); - - check("void f(int x) {\n" - " if (x >= 1.0 && x <= 1.001)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if (x < 1 && x > 3)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); - - check("void f(float x) {\n" - " if (x < 1.0 && x > 3.0)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1.0 && x > 3.0.\n", errout.str()); - - check("void f(int x) {\n" - " if (1 > x && 3 < x)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (x < 3 && x > 1)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if (x > 3 || x < 10)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x > 3 || x < 10.\n", errout.str()); - - check("void f(int x) {\n" - " if (x >= 3 || x <= 10)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x <= 10.\n", errout.str()); - - check("void f(int x) {\n" - " if (x >= 3 || x < 10)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x < 10.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 3 || x <= 10)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x > 3 || x <= 10.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 3 || x < 3)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if (x >= 3 || x <= 3)\n" - " a++;\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x <= 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (x >= 3 || x < 3)\n" - " a++;\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x >= 3 || x < 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 3 || x <= 3)\n" - " a++;\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x > 3 || x <= 3.\n", errout.str()); - - check("void f(int x) {\n" - " if((x==3) && (x!=4))\n" - " a++;\n" - "}\n" - ); - - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); - - check("void f(int x) {\n" - " if ((x!=4) && (x==3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); - - check("void f(int x) {\n" - " if ((x==3) || (x!=4))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); - - check("void f(int x) {\n" - " if ((x!=4) || (x==3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 3, the comparison x != 4 is always true.\n", errout.str()); - - check("void f(int x) {\n" - " if ((x==3) && (x!=3))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n", errout.str()); - - check("void f(int x) {\n" - " if ((x==6) || (x!=6))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x == 6 || x != 6.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 10 || x < 3)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if (x > 5 && x == 1)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 5 && x == 1.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 5 && x == 6)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 6, the comparison x > 5 is always true.\n", errout.str()); - - // #3419 - check("void f() {\n" - " if ( &q != &a && &q != &b ) { }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // #3676 - check("void f(int m_x2, int w, int x) {\n" - " if (x + w - 1 > m_x2 || m_x2 < 0 )\n" - " m_x2 = x + w - 1;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(float x) {\n" // x+1 => x - " if (x <= 1.0e20 && x >= -1.0e20) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(float x) {\n" // x+1 => x - " if (x >= 1.0e20 && x <= 1.0e21) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(float x) {\n" // x+1 => x - " if (x <= -1.0e20 && x >= -1.0e21) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void incorrectLogicOperator3() { - check("void f(int x, bool& b) {\n" - " b = x > 5 && x == 1;\n" - " c = x < 1 && x == 3;\n" - " d = x >= 5 && x == 1;\n" - " e = x <= 1 && x == 3;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 5 && x == 1.\n" - "[test.cpp:3]: (warning) Logical conjunction always evaluates to false: x < 1 && x == 3.\n" - "[test.cpp:4]: (warning) Logical conjunction always evaluates to false: x >= 5 && x == 1.\n" - "[test.cpp:5]: (warning) Logical conjunction always evaluates to false: x <= 1 && x == 3.\n", errout.str()); - } - - void incorrectLogicOperator4() { - check("void f(int x) {\n" - " if (x && x != $0) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void incorrectLogicOperator5() { // complex expressions - check("void f(int x) {\n" - " if (x+3 > 2 || x+3 < 10) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: EXPR > 2 || EXPR < 10.\n", errout.str()); - } - - void incorrectLogicOperator6() { // char literals - check("void f(char x) {\n" - " if (x == '1' || x == '2') {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(char x) {\n" - " if (x == '1' && x == '2') {}\n" - "}"); - TODO_ASSERT_EQUALS("error", "", errout.str()); - } - - void incorrectLogicOperator7() { // opposite expressions - check("void f(int i) {\n" - " if (i || !i) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: i||!i.\n", errout.str()); - } - - void secondAlwaysTrueFalseWhenFirstTrueError() { - check("void f(int x) {\n" - " if (x > 5 && x != 1)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 5 && x != 6)\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x) {\n" - " if ((x > 5) && (x != 1))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n", errout.str()); - - check("void f(int x) {\n" - " if ((x > 5) && (x != 6))\n" - " a++;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int x, bool& b) {\n" - " b = x > 3 || x == 4;\n" - " c = x < 5 || x == 4;\n" - " d = x >= 3 || x == 4;\n" - " e = x <= 5 || x == 4;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 4, the comparison x > 3 is always true.\n" - "[test.cpp:3]: (style) Redundant condition: If x == 4, the comparison x < 5 is always true.\n" - "[test.cpp:4]: (style) Redundant condition: If x == 4, the comparison x >= 3 is always true.\n" - "[test.cpp:5]: (style) Redundant condition: If x == 4, the comparison x <= 5 is always true.\n", errout.str()); - - check("void f(int x, bool& b) {\n" - " b = x > 5 || x != 1;\n" - " c = x < 1 || x != 3;\n" - " d = x >= 5 || x != 1;\n" - " e = x <= 1 || x != 3;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n" - "[test.cpp:3]: (style) Redundant condition: If x < 1, the comparison x != 3 is always true.\n" - "[test.cpp:4]: (style) Redundant condition: If x >= 5, the comparison x != 1 is always true.\n" - "[test.cpp:5]: (style) Redundant condition: If x <= 1, the comparison x != 3 is always true.\n", errout.str()); - - check("void f(int x, bool& b) {\n" - " b = x > 6 && x > 5;\n" - " c = x > 5 || x > 6;\n" - " d = x < 6 && x < 5;\n" - " e = x < 5 || x < 6;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 6, the comparison x > 5 is always true.\n" - "[test.cpp:3]: (style) Redundant condition: If x > 6, the comparison x > 5 is always true.\n" - "[test.cpp:4]: (style) Redundant condition: If x < 5, the comparison x < 6 is always true.\n" - "[test.cpp:5]: (style) Redundant condition: If x < 5, the comparison x < 6 is always true.\n", errout.str()); - } - - void incorrectLogicOp_condSwapping() { - check("void f(int x) {\n" - " if (x < 1 && x > 3)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (1 > x && x > 3)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (x < 1 && 3 < x)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (1 > x && 3 < x)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x < 1 && x > 3.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 3 && x < 1)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); - - check("void f(int x) {\n" - " if (3 < x && x < 1)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); - - check("void f(int x) {\n" - " if (x > 3 && 1 > x)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); - - check("void f(int x) {\n" - " if (3 < x && 1 > x)\n" - " a++;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 3 && x < 1.\n", errout.str()); - } - void memsetZeroBytes() { check("void f() {\n" " memset(p, 10, 0x0);\n" @@ -4395,97 +3670,6 @@ private: ASSERT_EQUALS("", errout.str()); } - // clarify conditions with = and comparison - void clarifyCondition1() { - check("void f() {\n" - " if (x = b() < 0) {}\n" // don't simplify and verify this code - "}", 0, false, false, false, false); - ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.\n", errout.str()); - - check("void f(int i) {\n" - " for (i = 0; i < 10; i++) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - " x = a(); if (x) {}\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - } - - // clarify conditions with bitwise operator and comparison - void clarifyCondition2() { - check("void f() {\n" - " if (x & 3 == 2) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n", errout.str()); - - check("void f() {\n" - " if (a & fred1.x == fred2.y) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n", errout.str()); - } - - // clarify condition that uses ! operator and then bitwise operator - void clarifyCondition3() { - check("void f(int w) {\n" - " if(!w & 0x8000) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n", errout.str()); - - check("void f() {\n" - " if (x == foo() & 2) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n", errout.str()); - - check("void f(std::list &ints) { }"); - ASSERT_EQUALS("", errout.str()); - - check("void f() { A a; }"); - ASSERT_EQUALS("", errout.str()); - - check("void f() { a(x there are never templates - ASSERT_EQUALS("[test.c:1]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n", errout.str()); - - check("class A;", "test.C"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - " if (result != (char *)&inline_result) { }\n" // don't simplify and verify cast - "}", 0, false, false, false, false); - ASSERT_EQUALS("", errout.str()); - } - - void clarifyCondition4() { // ticket #3110 - check("typedef double SomeType;\n" - "typedef std::pair PairType;\n" - "struct S\n" - "{\n" - " bool operator()\n" - " ( PairType const & left\n" - " , PairType const & right) const\n" - " {\n" - " return left.first < right.first;\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void clarifyCondition5() { // ticket #3609 (using | in template instantiation) - check("CWinTraits::GetWndStyle(0);"); - ASSERT_EQUALS("", errout.str()); - } - - void clarifyCondition6() { - check("template\n" - "SharedPtr& operator=( SharedPtr const & r ) {\n" - " px = r.px;\n" - " return *this;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - void duplicateBranch() { check("void f(int a, int &b) {\n" " if (a)\n" @@ -4754,6 +3938,12 @@ private: " if ((b + a) > (a + b)) {}\n" "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '>'.\n", errout.str()); + + check("void f(int x) {\n" + " if ((x == 1) && (x == 0x00000001))\n" + " a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&&'.\n", errout.str()); } void duplicateExpression2() { // check if float is NaN or Inf diff --git a/test/testrunner.vcxproj b/test/testrunner.vcxproj index aff333697..e1da25e88 100644 --- a/test/testrunner.vcxproj +++ b/test/testrunner.vcxproj @@ -32,7 +32,6 @@ - @@ -40,6 +39,7 @@ + diff --git a/test/testrunner.vcxproj.filters b/test/testrunner.vcxproj.filters index e4efdeae1..5b1700cd0 100644 --- a/test/testrunner.vcxproj.filters +++ b/test/testrunner.vcxproj.filters @@ -25,9 +25,6 @@ Source Files - - Source Files - Source Files @@ -190,6 +187,9 @@ Source Files + + Source Files +