Improved CheckOther::checkIncorrectLogicOperator:

- Implemented automatic swapping of conditions and operands
- Added several patterns
- Added support for conditions outside of if/while
This commit is contained in:
PKEuS 2012-03-12 19:06:30 +01:00
parent a9d56f2738
commit 7cfffc9c9d
2 changed files with 202 additions and 98 deletions

View File

@ -943,38 +943,86 @@ void CheckOther::assignmentInAssertError(const Token *tok, const std::string &va
// //
// Inform that second comparison is always true when first comparison is true. // Inform that second comparison is always true when first comparison is true.
//--------------------------------------------------------------------------- //---------------------------------------------------------------------------
enum Position { First, Second, NA };
enum Relation { Equal, NotEqual, Less, LessEqual, More, MoreEqual };
struct Condition {
Position position;
const char *opTokStr;
};
static std::string invertOperatorForOperandSwap(std::string s)
{
for (std::string::size_type i = 0; i < s.length(); i++) {
if (s[i] == '>')
s[i] = '<';
else if (s[i] == '<')
s[i] = '>';
}
return s;
}
static bool analyzeLogicOperatorCondition(const Condition& c1, const Condition& c2,
bool inv1, bool inv2,
bool varFirst1, bool varFirst2,
const std::string& firstConstant, const std::string& secondConstant,
const Token* op1Tok, const Token* op3Tok,
Relation relation)
{
if (!(c1.position == NA || (c1.position == First && varFirst1) || (c1.position == Second && !varFirst1)))
return false;
if (!(c2.position == NA || (c2.position == First && varFirst2) || (c2.position == Second && !varFirst2)))
return false;
if (!Token::Match(op1Tok, inv1?invertOperatorForOperandSwap(c1.opTokStr).c_str():c1.opTokStr))
return false;
if (!Token::Match(op3Tok, inv2?invertOperatorForOperandSwap(c2.opTokStr).c_str():c2.opTokStr))
return false;
return (relation == Equal && MathLib::isEqual(firstConstant, secondConstant)) ||
(relation == NotEqual && MathLib::isNotEqual(firstConstant, secondConstant)) ||
(relation == Less && MathLib::isLess(firstConstant, secondConstant)) ||
(relation == LessEqual && MathLib::isLessEqual(firstConstant, secondConstant)) ||
(relation == More && MathLib::isGreater(firstConstant, secondConstant)) ||
(relation == MoreEqual && MathLib::isGreaterEqual(firstConstant, secondConstant));
}
void CheckOther::checkIncorrectLogicOperator() void CheckOther::checkIncorrectLogicOperator()
{ {
if (!_settings->isEnabled("style")) if (!_settings->isEnabled("style"))
return; return;
const char conditionPattern[] = "if|while ("; for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) {
const Token *tok = Token::findmatch(_tokenizer->tokens(), conditionPattern);
const Token *endTok = tok ? tok->next()->link() : NULL;
while (tok && endTok) {
// Find a pair of comparison expressions with or without parenthesis // Find a pair of comparison expressions with or without parenthesis
// with a shared variable and constants and with a logical operator between them. // with a shared variable and constants and with a logical operator between them.
// e.g. if (x != 3 || x != 4) // e.g. if (x != 3 || x != 4)
const Token *logicTok = NULL, *term1Tok = NULL, *term2Tok = NULL; const Token *term1Tok = NULL, *term2Tok = NULL;
const Token *op1Tok = NULL, *op2Tok = NULL, *op3Tok = NULL, *nextTok = NULL; const Token *op1Tok = NULL, *op2Tok = NULL, *op3Tok = NULL, *nextTok = NULL;
if (NULL != (logicTok = Token::findmatch(tok, "( %any% !=|==|<|>|>=|<= %any% ) &&|%oror% ( %any% !=|==|<|>|>=|<= %any% ) %any%", endTok))) {
term1Tok = logicTok->next(); if (Token::Match(tok, "( %any% !=|==|<|>|>=|<= %any% ) &&|%oror%")) {
term2Tok = logicTok->tokAt(7); term1Tok = tok->next();
op1Tok = logicTok->tokAt(2); op1Tok = tok->tokAt(2);
op2Tok = logicTok->tokAt(5); op2Tok = tok->tokAt(5);
op3Tok = logicTok->tokAt(8); } else if (Token::Match(tok, "%any% !=|==|<|>|>=|<= %any% &&|%oror%")) {
nextTok = logicTok->tokAt(11); term1Tok = tok;
} else if (NULL != (logicTok = Token::findmatch(tok, "%any% !=|==|<|>|>=|<= %any% &&|%oror% %any% !=|==|<|>|>=|<= %any% %any%", endTok))) { op1Tok = tok->next();
term1Tok = logicTok; op2Tok = tok->tokAt(3);
term2Tok = logicTok->tokAt(4); }
op1Tok = logicTok->next(); if (op2Tok) {
op2Tok = logicTok->tokAt(3); if (Token::Match(op2Tok->next(), "( %any% !=|==|<|>|>=|<= %any% ) %any%")) {
op3Tok = logicTok->tokAt(5); term2Tok = op2Tok->tokAt(2);
nextTok = logicTok->tokAt(7); op3Tok = op2Tok->tokAt(3);
nextTok = op2Tok->tokAt(6);
} else if (Token::Match(op2Tok->next(), "%any% !=|==|<|>|>=|<= %any% %any%")) {
term2Tok = op2Tok->next();
op3Tok = op2Tok->tokAt(2);
nextTok = op2Tok->tokAt(4);
}
} }
if (logicTok) { if (nextTok) {
// Find the common variable and the two different-valued constants // Find the common variable and the two different-valued constants
unsigned int variableTested = 0; unsigned int variableTested = 0;
std::string firstConstant, secondConstant; std::string firstConstant, secondConstant;
@ -985,8 +1033,6 @@ void CheckOther::checkIncorrectLogicOperator()
varTok = term1Tok; varTok = term1Tok;
varId = varTok->varId(); varId = varTok->varId();
if (!varId) { if (!varId) {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
varFirst1 = true; varFirst1 = true;
@ -995,130 +1041,138 @@ void CheckOther::checkIncorrectLogicOperator()
varTok = term1Tok->tokAt(2); varTok = term1Tok->tokAt(2);
varId = varTok->varId(); varId = varTok->varId();
if (!varId) { if (!varId) {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
varFirst1 = false; varFirst1 = false;
firstConstant = term1Tok->str(); firstConstant = term1Tok->str();
} else { } else {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
if (Token::Match(term2Tok, "%var% %any% %num%")) { if (Token::Match(term2Tok, "%var% %any% %num%")) {
const unsigned int varId2 = term2Tok->varId(); const unsigned int varId2 = term2Tok->varId();
if (!varId2 || varId != varId2) { if (!varId2 || varId != varId2) {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
varFirst2 = true; varFirst2 = true;
secondConstant = term2Tok->strAt(2); secondConstant = term2Tok->strAt(2);
variableTested = varId; variableTested = varId;
} else if (Token::Match(term2Tok, "%num% %any% %var%")) { } else if (Token::Match(term2Tok, "%num% %any% %var%")) {
const unsigned int varId2 = term1Tok->tokAt(2)->varId(); const unsigned int varId2 = term2Tok->tokAt(2)->varId();
if (!varId2 || varId != varId2) { if (!varId2 || varId != varId2) {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
varFirst2 = false; varFirst2 = false;
secondConstant = term2Tok->str(); secondConstant = term2Tok->str();
variableTested = varId; variableTested = varId;
} else { } else {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
if (variableTested == 0 || firstConstant.empty() || secondConstant.empty()) { if (variableTested == 0 || firstConstant.empty() || secondConstant.empty()) {
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
continue; continue;
} }
enum Position { First, Second, NA }; enum LogicError { AlwaysFalse, AlwaysTrue, FirstTrue, FirstFalse, SecondTrue, SecondFalse };
enum Relation { Equal, NotEqual, Less, LessEqual, More, MoreEqual };
enum LogicError { Exclusion, AlwaysTrue, AlwaysFalse, AlwaysFalseOr }; static const struct LinkedConditions {
static const struct Condition {
const char *before; const char *before;
Position position1; Condition c1;
const char *op1TokStr;
const char *op2TokStr; const char *op2TokStr;
Position position2; Condition c2;
const char *op3TokStr;
const char *after; const char *after;
Relation relation; Relation relation;
LogicError error; LogicError error;
} conditions[] = { } conditions[] = {
{ "!!&&", NA, "!=", "||", NA, "!=", "!!&&", NotEqual, Exclusion }, // (x != 1) || (x != 3) <- always true { "!!&&", { NA, "!=" }, "%oror%", { NA, "!=" }, "!!&&", NotEqual, AlwaysTrue }, // (x != 1) || (x != 3) <- always true
{ "(", NA, "==", "&&", NA, "==", ")", NotEqual, AlwaysFalseOr }, // (x == 1) && (x == 3) <- always false { 0, { NA, "==" }, "&&", { NA, "==" }, 0, NotEqual, AlwaysFalse }, // (x == 1) && (x == 3) <- always false
{ "(", First, "<", "&&", First, ">", ")", LessEqual, AlwaysFalseOr }, // (x < 1) && (x > 3) <- always false { 0, { First, "<" }, "&&", { First, ">" }, 0, LessEqual, AlwaysFalse }, // (x < 1) && (x > 3) <- always false
{ "(", First, ">", "&&", First, "<", ")", MoreEqual, AlwaysFalseOr }, // (x > 3) && (x < 1) <- always false { 0, { First, "<=" }, "&&", { First, ">|>=" }, 0, LessEqual, AlwaysFalse }, // (x <= 1) && (x > 3) <- always false
{ "(", Second, ">", "&&", First, ">", ")", LessEqual, AlwaysFalseOr }, // (1 > x) && (x > 3) <- always false { 0, { First, "<" }, "&&", { First, ">=" }, 0, LessEqual, AlwaysFalse }, // (x < 1) && (x >= 3) <- always false
{ "(", First, ">", "&&", Second, ">", ")", MoreEqual, AlwaysFalseOr }, // (x > 3) && (1 > x) <- always false { "!!&&", { First , ">" }, "%oror%", { First, "<" }, "!!&&", Less, AlwaysTrue }, // (x > 3) || (x < 10) <- always true
{ "(", First, "<", "&&", Second, "<", ")", LessEqual, AlwaysFalseOr }, // (x < 1) && (3 < x) <- always false { "!!&&", { First , ">=" }, "%oror%", { First, "<|<=" }, "!!&&", LessEqual, AlwaysTrue }, // (x >= 3) || (x < 10) <- always true
{ "(", Second, "<", "&&", First, "<", ")", MoreEqual, AlwaysFalseOr }, // (3 < x) && (x < 1) <- always false { "!!&&", { First , ">" }, "%oror%", { First, "<=" }, "!!&&", LessEqual, AlwaysTrue }, // (x > 3) || (x <= 10) <- always true
{ "(", Second, ">", "&&", Second, "<", ")", LessEqual, AlwaysFalseOr }, // (1 > x) && (3 < x) <- always false { 0, { First, ">" }, "&&", { NA, "!=" }, 0, MoreEqual, SecondTrue }, // (x > 5) && (x != 1) <- second expression always true
{ "(", Second, "<", "&&", Second, ">", ")", MoreEqual, AlwaysFalseOr }, // (3 < x) && (1 > x) <- always false { 0, { First, "<" }, "&&", { NA, "!=" }, 0, LessEqual, SecondTrue }, // (x < 1) && (x != 3) <- second expression always true
{ "(", First , ">|>=", "||", First, "<|<=", ")", Less, Exclusion }, // (x > 3) || (x < 10) <- always true { 0, { First, ">=" }, "&&", { NA, "!=" }, 0, More, SecondTrue }, // (x >= 5) && (x != 1) <- second expression always true
{ "(", First , "<|<=", "||", First, ">|>=", ")", More, Exclusion }, // (x < 10) || (x > 3) <- always true { 0, { First, "<=" }, "&&", { NA, "!=" }, 0, Less, SecondTrue }, // (x <= 1) && (x != 3) <- second expression always true
{ "(", Second, "<|<=", "||", First, "<|<=", ")", Less, Exclusion }, // (3 < x) || (x < 10) <- always true { 0, { First, ">" }, "&&", { NA, "==" }, 0, MoreEqual, SecondFalse }, // (x > 5) && (x == 1) <- second expression always false
{ "(", First, "<|<=", "||", Second, "<|<=", ")", More, Exclusion }, // (x < 10) || (3 < x) <- always true { 0, { First, "<" }, "&&", { NA, "==" }, 0, LessEqual, SecondFalse }, // (x < 1) && (x == 3) <- second expression always false
{ "(", First, ">|>=", "||", Second, ">|>=", ")", Less, Exclusion }, // (x > 3) || (10 > x) <- always true { 0, { First, ">=" }, "&&", { NA, "==" }, 0, More, SecondFalse }, // (x >= 5) && (x == 1) <- second expression always false
{ "(", Second, ">|>=", "||", First, ">|>=", ")", More, Exclusion }, // (10 > x) || (x > 3) <- always true { 0, { First, "<=" }, "&&", { NA, "==" }, 0, Less, SecondFalse }, // (x <= 1) && (x == 3) <- second expression always false
{ "(", Second, "<|<=", "||", Second, ">|<=", ")", Less, Exclusion }, // (3 < x) || (10 > x) <- always true
{ "(", Second, ">|>=", "||", Second, "<|<=", ")", More, Exclusion }, // (10 > x) || (3 < x) <- always true
{ "(", First, ">", "&&", NA, "!=", ")", More, AlwaysTrue }, // (x > 5) && (x != 1) <- second expression always true
{ "(", Second, "<", "&&", NA, "!=", ")", More, AlwaysTrue }, // (5 < x) && (x != 1) <- second expression always true
{ "(", First, ">", "&&", NA, "==", ")", More, AlwaysFalse }, // (x > 5) && (x == 1) <- second expression always false
{ "(", Second, "<", "&&", NA, "==", ")", More, AlwaysFalse }, // (5 < x) && (x == 1) <- second expression always false
}; };
for (unsigned int i = 0; i < (sizeof(conditions) / sizeof(conditions[0])); i++) { for (unsigned int i = 0; i < (sizeof(conditions) / sizeof(conditions[0])); i++) {
if (!((conditions[i].position1 == NA) || (((conditions[i].position1 == First) && varFirst1) || ((conditions[i].position1 == Second) && !varFirst1))))
continue;
if (!((conditions[i].position2 == NA) || (((conditions[i].position2 == First) && varFirst2) || ((conditions[i].position2 == Second) && !varFirst2))))
continue;
if (!Token::Match(op1Tok, conditions[i].op1TokStr))
continue;
if (!Token::Match(op2Tok, conditions[i].op2TokStr)) if (!Token::Match(op2Tok, conditions[i].op2TokStr))
continue; continue;
if (!Token::Match(op3Tok, conditions[i].op3TokStr)) if (conditions[i].before != 0 && !Token::Match(tok->previous(), conditions[i].before))
continue; continue;
if (!Token::Match(logicTok->previous(), conditions[i].before)) if (conditions[i].after != 0 && !Token::Match(nextTok, conditions[i].after))
continue; continue;
if (!Token::Match(nextTok, conditions[i].after)) // cond1 op cond2
continue; bool error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, false, false,
varFirst1, varFirst2, firstConstant, secondConstant,
op1Tok, op3Tok,
conditions[i].relation);
// inv(cond1) op cond2 // invert first condition
if (!error && conditions[i].c1.position != NA)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, true, false,
!varFirst1, varFirst2, firstConstant, secondConstant,
op1Tok, op3Tok,
conditions[i].relation);
// cond1 op inv(cond2) // invert second condition
if (!error && conditions[i].c2.position != NA)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, false, true,
varFirst1, !varFirst2, firstConstant, secondConstant,
op1Tok, op3Tok,
conditions[i].relation);
// inv(cond1) op inv(cond2) // invert both conditions
if (!error && conditions[i].c1.position != NA && conditions[i].c2.position != NA)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, true, true,
!varFirst1, !varFirst2, firstConstant, secondConstant,
op1Tok, op3Tok,
conditions[i].relation);
// cond2 op cond1 // swap conditions
if (!error)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, false, false,
varFirst2, varFirst1, secondConstant, firstConstant,
op3Tok, op1Tok,
conditions[i].relation);
// cond2 op inv(cond1) // swap conditions; invert first condition
if (!error && conditions[i].c1.position != NA)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, true, false,
!varFirst2, varFirst1, secondConstant, firstConstant,
op3Tok, op1Tok,
conditions[i].relation);
// inv(cond2) op cond1 // swap conditions; invert second condition
if (!error && conditions[i].c2.position != NA)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, false, true,
varFirst2, !varFirst1, secondConstant, firstConstant,
op3Tok, op1Tok,
conditions[i].relation);
// inv(cond2) op inv(cond1) // swap conditions; invert both conditions
if (!error && conditions[i].c1.position != NA && conditions[i].c2.position != NA)
error = analyzeLogicOperatorCondition(conditions[i].c1, conditions[i].c2, true, true,
!varFirst2, !varFirst1, secondConstant, firstConstant,
op3Tok, op1Tok,
conditions[i].relation);
if ((conditions[i].relation == Equal && MathLib::isEqual(firstConstant, secondConstant)) || if (error) {
(conditions[i].relation == NotEqual && MathLib::isNotEqual(firstConstant, secondConstant)) || if (conditions[i].error == AlwaysFalse || conditions[i].error == AlwaysTrue)
(conditions[i].relation == Less && MathLib::isLess(firstConstant, secondConstant)) || incorrectLogicOperatorError(term1Tok, conditions[i].error == AlwaysTrue);
(conditions[i].relation == LessEqual && MathLib::isLessEqual(firstConstant, secondConstant)) ||
(conditions[i].relation == More && MathLib::isGreater(firstConstant, secondConstant)) ||
(conditions[i].relation == MoreEqual && MathLib::isGreaterEqual(firstConstant, secondConstant))) {
if (conditions[i].error == Exclusion || conditions[i].error == AlwaysFalseOr)
incorrectLogicOperatorError(term1Tok, conditions[i].error == Exclusion);
else { else {
std::string text("When " + varTok->str() + " is greater than " + firstConstant + ", the comparison " + std::string text("When " + varTok->str() + " is greater than " + firstConstant + ", the comparison " +
varTok->str() + " " + conditions[i].op3TokStr + " " + secondConstant + varTok->str() + " " + conditions[i].c2.opTokStr + " " + secondConstant +
" is always " + (conditions[i].error == AlwaysTrue ? "true." : "false.")); " is always " + (conditions[i].error == SecondTrue ? "true." : "false."));
secondAlwaysTrueFalseWhenFirstTrueError(term1Tok, text); secondAlwaysTrueFalseWhenFirstTrueError(term1Tok, text);
} }
break;
} }
} }
} }
tok = Token::findmatch(endTok->next(), conditionPattern);
endTok = tok ? tok->next()->link() : NULL;
} }
} }

View File

@ -114,6 +114,7 @@ private:
TEST_CASE(incorrectLogicOperator1); TEST_CASE(incorrectLogicOperator1);
TEST_CASE(incorrectLogicOperator2); TEST_CASE(incorrectLogicOperator2);
TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError); TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError);
TEST_CASE(incorrectLogicOp_condSwapping);
TEST_CASE(memsetZeroBytes); TEST_CASE(memsetZeroBytes);
@ -2859,23 +2860,23 @@ private:
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x >= 3 || x <= 3)\n" " if (x >= 3 || x <= 3)\n"
" a++;\n" " a++;\n"
"}\n" "}"
); );
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x >= 3 || x < 3)\n" " if (x >= 3 || x < 3)\n"
" a++;\n" " a++;\n"
"}\n" "}"
); );
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x > 3 || x <= 3)\n" " if (x > 3 || x <= 3)\n"
" a++;\n" " a++;\n"
"}\n" "}"
); );
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x > 10 || x < 3)\n" " if (x > 10 || x < 3)\n"
@ -3005,6 +3006,55 @@ private:
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", 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) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (1 > x && x > 3)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (x < 1 && 3 < x)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (1 > x && 3 < x)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (x > 3 && x < 1)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (3 < x && x < 1)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (x > 3 && 1 > x)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
check("void f(int x) {\n"
" if (3 < x && 1 > x)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Expression always evaluates to false. Did you intend to use || instead?\n", errout.str());
}
void comparisonOfBoolExpressionWithInt() { void comparisonOfBoolExpressionWithInt() {
check("void f(int x) {\n" check("void f(int x) {\n"