Add a check for duplicate if statements
This will warn for this: ```cpp int f(int val) { int i = 0; if( val & 0xff) i = 1; if( val & 0xff) i = 1; return i; } ```
This commit is contained in:
parent
35e56942d1
commit
4e147a4c59
|
@ -415,6 +415,63 @@ bool CheckCondition::isOverlappingCond(const Token * const cond1, const Token *
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void CheckCondition::duplicateCondition()
|
||||||
|
{
|
||||||
|
if (!mSettings->isEnabled(Settings::STYLE))
|
||||||
|
return;
|
||||||
|
|
||||||
|
const SymbolDatabase *const symbolDatabase = mTokenizer->getSymbolDatabase();
|
||||||
|
|
||||||
|
for (const Scope &scope : symbolDatabase->scopeList) {
|
||||||
|
if (scope.type != Scope::eIf)
|
||||||
|
continue;
|
||||||
|
|
||||||
|
const Token *cond1 = scope.classDef->next()->astOperand2();
|
||||||
|
if (!cond1)
|
||||||
|
continue;
|
||||||
|
if (cond1->hasKnownIntValue())
|
||||||
|
continue;
|
||||||
|
|
||||||
|
const Token *tok2 = scope.classDef->next();
|
||||||
|
if (!tok2)
|
||||||
|
continue;
|
||||||
|
tok2 = tok2->link();
|
||||||
|
if (!Token::simpleMatch(tok2, ") {"))
|
||||||
|
continue;
|
||||||
|
tok2 = tok2->linkAt(1);
|
||||||
|
if (!Token::simpleMatch(tok2, "} if ("))
|
||||||
|
continue;
|
||||||
|
const Token *cond2 = tok2->tokAt(2)->astOperand2();
|
||||||
|
if (!cond2)
|
||||||
|
continue;
|
||||||
|
|
||||||
|
bool modified = false;
|
||||||
|
visitAstNodes(cond1, [&](const Token *tok3) {
|
||||||
|
if (tok3->varId() > 0 &&
|
||||||
|
isVariableChanged(scope.classDef->next(), cond2, tok3->varId(), false, mSettings, mTokenizer->isCPP())) {
|
||||||
|
modified = true;
|
||||||
|
return ChildrenToVisit::done;
|
||||||
|
}
|
||||||
|
return ChildrenToVisit::op1_and_op2;
|
||||||
|
});
|
||||||
|
ErrorPath errorPath;
|
||||||
|
if (!modified &&
|
||||||
|
isSameExpression(mTokenizer->isCPP(), true, cond1, cond2, mSettings->library, true, true, &errorPath))
|
||||||
|
duplicateConditionError(cond1, cond2, errorPath);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
void CheckCondition::duplicateConditionError(const Token *tok1, const Token *tok2, ErrorPath errorPath)
|
||||||
|
{
|
||||||
|
if (diag(tok1) & diag(tok2))
|
||||||
|
return;
|
||||||
|
errorPath.emplace_back(tok1, "First condition");
|
||||||
|
errorPath.emplace_back(tok2, "Second condition");
|
||||||
|
|
||||||
|
std::string msg = "The if condition is the same as the previous if condition";
|
||||||
|
|
||||||
|
reportError(errorPath, Severity::style, "duplicateCondition", msg, CWE398, false);
|
||||||
|
}
|
||||||
|
|
||||||
void CheckCondition::multiCondition()
|
void CheckCondition::multiCondition()
|
||||||
{
|
{
|
||||||
|
|
|
@ -59,6 +59,7 @@ public:
|
||||||
checkCondition.checkIncorrectLogicOperator();
|
checkCondition.checkIncorrectLogicOperator();
|
||||||
checkCondition.checkInvalidTestForOverflow();
|
checkCondition.checkInvalidTestForOverflow();
|
||||||
checkCondition.alwaysTrueFalse();
|
checkCondition.alwaysTrueFalse();
|
||||||
|
checkCondition.duplicateCondition();
|
||||||
checkCondition.checkPointerAdditionResultNotNull();
|
checkCondition.checkPointerAdditionResultNotNull();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -88,6 +89,8 @@ public:
|
||||||
/** mismatching lhs and rhs in comparison */
|
/** mismatching lhs and rhs in comparison */
|
||||||
void comparison();
|
void comparison();
|
||||||
|
|
||||||
|
void duplicateCondition();
|
||||||
|
|
||||||
/** match 'if' and 'else if' conditions */
|
/** match 'if' and 'else if' conditions */
|
||||||
void multiCondition();
|
void multiCondition();
|
||||||
|
|
||||||
|
@ -132,6 +135,7 @@ private:
|
||||||
const std::string &op,
|
const std::string &op,
|
||||||
MathLib::bigint value2,
|
MathLib::bigint value2,
|
||||||
bool result);
|
bool result);
|
||||||
|
void duplicateConditionError(const Token *tok1, const Token *tok2, ErrorPath errorPath);
|
||||||
void multiConditionError(const Token *tok, unsigned int line1);
|
void multiConditionError(const Token *tok, unsigned int line1);
|
||||||
|
|
||||||
void oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath);
|
void oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath);
|
||||||
|
@ -160,6 +164,7 @@ private:
|
||||||
c.assignIfError(nullptr, nullptr, emptyString, false);
|
c.assignIfError(nullptr, nullptr, emptyString, false);
|
||||||
c.badBitmaskCheckError(nullptr);
|
c.badBitmaskCheckError(nullptr);
|
||||||
c.comparisonError(nullptr, "&", 6, "==", 1, false);
|
c.comparisonError(nullptr, "&", 6, "==", 1, false);
|
||||||
|
c.duplicateConditionError(nullptr, nullptr, errorPath);
|
||||||
c.multiConditionError(nullptr,1);
|
c.multiConditionError(nullptr,1);
|
||||||
c.mismatchingBitAndError(nullptr, 0xf0, nullptr, 1);
|
c.mismatchingBitAndError(nullptr, 0xf0, nullptr, 1);
|
||||||
c.oppositeInnerConditionError(nullptr, nullptr, errorPath);
|
c.oppositeInnerConditionError(nullptr, nullptr, errorPath);
|
||||||
|
|
|
@ -107,6 +107,7 @@ private:
|
||||||
|
|
||||||
TEST_CASE(alwaysTrue);
|
TEST_CASE(alwaysTrue);
|
||||||
TEST_CASE(multiConditionAlwaysTrue);
|
TEST_CASE(multiConditionAlwaysTrue);
|
||||||
|
TEST_CASE(duplicateCondition);
|
||||||
|
|
||||||
TEST_CASE(checkInvalidTestForOverflow);
|
TEST_CASE(checkInvalidTestForOverflow);
|
||||||
TEST_CASE(checkConditionIsAlwaysTrueOrFalseInsideIfWhile);
|
TEST_CASE(checkConditionIsAlwaysTrueOrFalseInsideIfWhile);
|
||||||
|
@ -1792,53 +1793,83 @@ private:
|
||||||
" if (x<4) {\n"
|
" if (x<4) {\n"
|
||||||
" if (x==5) {}\n" // <- Warning
|
" if (x==5) {}\n" // <- Warning
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n",
|
||||||
|
errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<4) {\n"
|
" if (x<4) {\n"
|
||||||
" if (x!=5) {}\n" // <- TODO
|
" if (x!=5) {}\n" // <- TODO
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<4) {\n"
|
" if (x<4) {\n"
|
||||||
" if (x>5) {}\n" // <- Warning
|
" if (x>5) {}\n" // <- Warning
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n",
|
||||||
|
errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<4) {\n"
|
" if (x<4) {\n"
|
||||||
" if (x>=5) {}\n" // <- Warning
|
" if (x>=5) {}\n" // <- Warning
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n",
|
||||||
|
errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<4) {\n"
|
" if (x<4) {\n"
|
||||||
" if (x<5) {}\n"
|
" if (x<5) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<4) {\n"
|
" if (x<4) {\n"
|
||||||
" if (x<=5) {}\n"
|
" if (x<=5) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
"}");
|
"}");
|
||||||
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
|
ASSERT_EQUALS("", errout.str());
|
||||||
"[test.cpp:11] -> [test.cpp:12]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
|
|
||||||
"[test.cpp:15] -> [test.cpp:16]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
|
|
||||||
, errout.str());
|
|
||||||
|
|
||||||
check("void f(int x) {\n"
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<5) {\n"
|
" if (x<5) {\n"
|
||||||
" if (x==4) {}\n"
|
" if (x==4) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<5) {\n"
|
" if (x<5) {\n"
|
||||||
" if (x!=4) {}\n"
|
" if (x!=4) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<5) {\n"
|
" if (x<5) {\n"
|
||||||
" if (x>4) {}\n" // <- TODO
|
" if (x>4) {}\n" // <- TODO
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<5) {\n"
|
" if (x<5) {\n"
|
||||||
" if (x>=4) {}\n"
|
" if (x>=4) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<5) {\n"
|
" if (x<5) {\n"
|
||||||
" if (x<4) {}\n"
|
" if (x<4) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x<5) {\n"
|
" if (x<5) {\n"
|
||||||
" if (x<=4) {}\n"
|
" if (x<=4) {}\n"
|
||||||
|
@ -1852,18 +1883,30 @@ private:
|
||||||
" if (x>4) {\n"
|
" if (x>4) {\n"
|
||||||
" if (x==5) {}\n"
|
" if (x==5) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>4) {\n"
|
" if (x>4) {\n"
|
||||||
" if (x>5) {}\n"
|
" if (x>5) {}\n"
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>4) {\n"
|
" if (x>4) {\n"
|
||||||
" if (x>=5) {}\n" // <- TODO
|
" if (x>=5) {}\n" // <- TODO
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>4) {\n"
|
" if (x>4) {\n"
|
||||||
" if (x<5) {}\n" // <- TODO
|
" if (x<5) {}\n" // <- TODO
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>4) {\n"
|
" if (x>4) {\n"
|
||||||
" if (x<=5) {}\n"
|
" if (x<=5) {}\n"
|
||||||
|
@ -1876,27 +1919,39 @@ private:
|
||||||
" if (x>5) {\n"
|
" if (x>5) {\n"
|
||||||
" if (x==4) {}\n" // <- Warning
|
" if (x==4) {}\n" // <- Warning
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n",
|
||||||
|
errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>5) {\n"
|
" if (x>5) {\n"
|
||||||
" if (x>4) {}\n" // <- TODO
|
" if (x>4) {}\n" // <- TODO
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>5) {\n"
|
" if (x>5) {\n"
|
||||||
" if (x>=4) {}\n" // <- TODO
|
" if (x>=4) {}\n" // <- TODO
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>5) {\n"
|
" if (x>5) {\n"
|
||||||
" if (x<4) {}\n" // <- Warning
|
" if (x<4) {}\n" // <- Warning
|
||||||
" }\n"
|
" }\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n",
|
||||||
|
errout.str());
|
||||||
|
check("void f(int x) {\n"
|
||||||
"\n"
|
"\n"
|
||||||
" if (x>5) {\n"
|
" if (x>5) {\n"
|
||||||
" if (x<=4) {}\n" // <- Warning
|
" if (x<=4) {}\n" // <- Warning
|
||||||
" }\n"
|
" }\n"
|
||||||
"}");
|
"}");
|
||||||
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
|
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n",
|
||||||
"[test.cpp:15] -> [test.cpp:16]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
|
errout.str());
|
||||||
"[test.cpp:19] -> [test.cpp:20]: (warning) Opposite inner 'if' condition leads to a dead code block.\n"
|
|
||||||
, errout.str());
|
|
||||||
|
|
||||||
check("void f(int x) {\n"
|
check("void f(int x) {\n"
|
||||||
" if (x < 4) {\n"
|
" if (x < 4) {\n"
|
||||||
|
@ -2755,6 +2810,47 @@ private:
|
||||||
"[test.cpp:5]: (style) Condition 'foo' is always false\n", errout.str());
|
"[test.cpp:5]: (style) Condition 'foo' is always false\n", errout.str());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void duplicateCondition() {
|
||||||
|
check("void f(bool x) {\n"
|
||||||
|
" if(x) {}\n"
|
||||||
|
" if(x) {}\n"
|
||||||
|
"}\n");
|
||||||
|
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The if condition is the same as the previous if condition\n",
|
||||||
|
errout.str());
|
||||||
|
|
||||||
|
check("void f(int x) {\n"
|
||||||
|
" if(x == 1) {}\n"
|
||||||
|
" if(x == 1) {}\n"
|
||||||
|
"}\n");
|
||||||
|
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The if condition is the same as the previous if condition\n",
|
||||||
|
errout.str());
|
||||||
|
|
||||||
|
check("void f(int x) {\n"
|
||||||
|
" if(x == 1) {}\n"
|
||||||
|
" if(x == 2) {}\n"
|
||||||
|
"}\n");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
|
||||||
|
check("void f(int x) {\n"
|
||||||
|
" if(x == 1) {}\n"
|
||||||
|
" if(x != 1) {}\n"
|
||||||
|
"}\n");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
|
||||||
|
check("void f(bool x) {\n"
|
||||||
|
" if(x) {}\n"
|
||||||
|
" g();\n"
|
||||||
|
" if(x) {}\n"
|
||||||
|
"}\n");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
|
||||||
|
check("void f(int x) {\n"
|
||||||
|
" if(x == 1) { x++; }\n"
|
||||||
|
" if(x == 1) {}\n"
|
||||||
|
"}\n");
|
||||||
|
ASSERT_EQUALS("", errout.str());
|
||||||
|
}
|
||||||
|
|
||||||
void checkInvalidTestForOverflow() {
|
void checkInvalidTestForOverflow() {
|
||||||
check("void f(char *p, unsigned int x) {\n"
|
check("void f(char *p, unsigned int x) {\n"
|
||||||
" assert((p + x) < p);\n"
|
" assert((p + x) < p);\n"
|
||||||
|
|
Loading…
Reference in New Issue