From 215cb5ac8d6485e1f8aa10e8d327d65456525c45 Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Sun, 21 Nov 2010 00:06:43 -0800 Subject: [PATCH] Fixed #2162 (false positive: Mutual exclusion over ||) --- cli/cmdlineparser.cpp | 4 +-- lib/checkother.cpp | 67 ++++++++++++++++++++++++++++--------------- lib/checkother.h | 2 +- test/testother.cpp | 11 ++++++- 4 files changed, 57 insertions(+), 27 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index e12ccda24..913545f6d 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -293,8 +293,8 @@ bool CmdLineParser::ParseFromArgs(int argc, const char* const argv[]) if (_settings->_jobs > 10000) { - // This limit is here just to catch typos. If someone has - // need for more jobs, this value should be increased. + // This limit is here just to catch typos. If someone has + // need for more jobs, this value should be increased. PrintMessage("cppcheck: argument for '-j' is allowed to be 10000 at max"); return false; } diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 33d3d3de3..3a7731d1f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -220,10 +220,6 @@ void CheckOther::checkAssignmentInAssert() //--------------------------------------------------------------------------- void CheckOther::checkIncorrectLogicOperator() { - // Inconclusive until #2162 is fixed: - if (!_settings->inconclusive) - return; - if (!_settings->_checkCodingStyle) return; @@ -234,6 +230,7 @@ void CheckOther::checkIncorrectLogicOperator() while (tok && endTok) { // Find a pair of OR'd terms, with or without parenthesis + // e.g. if (x != 3 || x != 4) const Token *logicTok = NULL, *term1Tok = NULL, *term2Tok = NULL; if (NULL != (logicTok = Token::findmatch(tok, "( %any% != %any% ) %oror% ( %any% != %any% ) !!&&", endTok))) { @@ -246,29 +243,55 @@ void CheckOther::checkIncorrectLogicOperator() term2Tok = logicTok->tokAt(4); } - // If both terms reference a common variable and are not AND'd with anything, this is an error + // The terms must not be AND'd with anything, to prevent false positives if (logicTok && (logicTok->strAt(-1) != "&&")) { - // (var11 != var12) || (var21 != var22) - const unsigned int varId11 = term1Tok->varId(); - const unsigned int varId12 = term1Tok->tokAt(2)->varId(); - const unsigned int varId21 = term2Tok->varId(); - const unsigned int varId22 = term2Tok->tokAt(2)->varId(); + // Find the common variable and the two different-valued constants + unsigned int variableTested = 0; + std::string firstConstant, secondConstant; + if (Token::Match(term1Tok, "%var% != %num%")) + { + const unsigned int varId = term1Tok->varId(); + firstConstant = term1Tok->tokAt(2)->str(); - // (var != const1) || (var != const2) - if (Token::Match(term1Tok, "%var%") && - varId11 != 0 && - (varId11 == varId21 || varId11 == varId22)) + if (Token::Match(term2Tok, "%varid% != %num%", varId)) + { + variableTested = varId; + secondConstant = term2Tok->tokAt(2)->str(); + } + else if (Token::Match(term2Tok, "%num% != %varid%", varId)) + { + variableTested = varId; + secondConstant = term2Tok->str(); + } + } + else if (Token::Match(term1Tok, "%num% != %var%")) + { + const unsigned int varId = term1Tok->tokAt(2)->varId(); + firstConstant = term1Tok->str(); + + if (Token::Match(term2Tok, "%varid% != %num%", varId)) + { + variableTested = varId; + secondConstant = term2Tok->tokAt(2)->str(); + } + else if (Token::Match(term2Tok, "%num% != %varid%", varId)) + { + variableTested = varId; + secondConstant = term2Tok->str(); + } + } + + // If there is a common variable tested for inequality against + // either of two different-valued constants, then the expression + // will always evaluate to true and the || probably should be an && + if (variableTested != 0 && + !firstConstant.empty() && + !secondConstant.empty() && + firstConstant != secondConstant) { incorrectLogicOperatorError(term1Tok); } - // (const1 != var) || (const2 != var) - else if (Token::Match(term1Tok->tokAt(2), "%var%") && - varId12 != 0 && - (varId12 == varId21 || varId12 == varId22)) - { - incorrectLogicOperatorError(term1Tok->tokAt(2)); - } } tok = Token::findmatch(endTok->next(), conditionPattern); @@ -2643,8 +2666,6 @@ void CheckOther::assignmentInAssertError(const Token *tok, const std::string &va void CheckOther::incorrectLogicOperatorError(const Token *tok) { - if (!_settings->inconclusive) - return; reportError(tok, Severity::warning, "incorrectLogicOperator", "Mutual exclusion over || always evaluates to true. Did you intend to use && instead?"); } diff --git a/lib/checkother.h b/lib/checkother.h index da5f2e78c..bc4af9f10 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -251,7 +251,7 @@ public: "* look for 'sizeof sizeof ..'\n" "* look for calculations inside sizeof()\n" "* assignment of a variable to itself\n" - //"* mutual exclusion over || always evaluating to true\n" + "* mutual exclusion over || always evaluating to true\n" // optimisations "* optimisation: detect post increment/decrement\n"; diff --git a/test/testother.cpp b/test/testother.cpp index 9ff6c5175..b90b641eb 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -104,7 +104,6 @@ private: // Check.. Settings settings; settings._checkCodingStyle = true; - settings.inconclusive = true; CheckOther checkOther(&tokenizer, &settings, this); // Clear the error buffer.. @@ -1485,6 +1484,16 @@ private: "}\n" ); ASSERT_EQUALS("", 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()); } };