Skip to content

Commit 18d6c1d

Browse files
chrchr-githubchrchr-github
authored
Fix #14915 FN duplicateConditionalAssign with bool and explicit check (#8827)
1 parent f9ca1b4 commit 18d6c1d

3 files changed

Lines changed: 87 additions & 21 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1846,6 +1846,42 @@ void CheckConditionImpl::pointerAdditionResultNotNullError(const Token *tok, con
18461846
reportError(tok, Severity::warning, "pointerAdditionResultNotNull", "Comparison is wrong. Result of '" + s + "' can't be 0 unless there is pointer overflow, and pointer overflow is undefined behaviour.");
18471847
}
18481848

1849+
static bool checkBoolConditionalAssign(const Token* condTok, const Token* assignTok, bool& isRedundant)
1850+
{
1851+
bool isNegation = false;
1852+
const Token* varTok = condTok;
1853+
if (condTok->isUnaryOp("!")) {
1854+
isNegation = true;
1855+
varTok = varTok->astOperand1();
1856+
} else if (condTok->isBinaryOp()) {
1857+
varTok = condTok->astOperand1();
1858+
if (varTok->hasKnownIntValue())
1859+
varTok = condTok->astOperand2();
1860+
}
1861+
1862+
const ValueType* vt = varTok->variable() ? varTok->variable()->valueType() : nullptr;
1863+
if (!(vt && vt->type == ValueType::Type::BOOL && !vt->pointer))
1864+
return false;
1865+
1866+
if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == varTok->varId()))
1867+
return false;
1868+
if (!(assignTok->astOperand2() && assignTok->astOperand2()->hasKnownIntValue()))
1869+
return false;
1870+
const MathLib::bigint val = assignTok->astOperand2()->getKnownIntValue();
1871+
if (val < 0 || val > 1)
1872+
return false;
1873+
if (condTok->isBinaryOp()) {
1874+
if (!varTok->astSibling()->hasKnownIntValue())
1875+
return false;
1876+
const MathLib::bigint compVal = varTok->astSibling()->getKnownIntValue();
1877+
if (compVal < 0 || compVal > 1)
1878+
return false;
1879+
isNegation = (condTok->str() == "!=") == (compVal == 1);
1880+
}
1881+
isRedundant = (isNegation && val == 0) || (!isNegation && val == 1);
1882+
return true;
1883+
}
1884+
18491885
void CheckConditionImpl::checkDuplicateConditionalAssign()
18501886
{
18511887
if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("duplicateConditionalAssign"))
@@ -1862,7 +1898,7 @@ void CheckConditionImpl::checkDuplicateConditionalAssign()
18621898
continue;
18631899
const Token *blockTok = tok->linkAt(1)->next();
18641900
const Token *condTok = tok->next()->astOperand2();
1865-
const bool isBoolVar = Token::Match(condTok, "!| %var%");
1901+
bool isBoolVar = Token::Match(condTok, "!| %var%");
18661902
if (!isBoolVar && !Token::Match(condTok, "==|!="))
18671903
continue;
18681904
if ((isBoolVar || condTok->str() == "!=") && Token::simpleMatch(blockTok->link(), "} else {"))
@@ -1875,21 +1911,8 @@ void CheckConditionImpl::checkDuplicateConditionalAssign()
18751911
if (nextAfterAstRightmostLeaf(assignTok) != blockTok->link()->previous())
18761912
continue;
18771913
bool isRedundant = false;
1878-
if (isBoolVar) {
1879-
const bool isNegation = condTok->str() == "!";
1880-
const Token* const varTok = isNegation ? condTok->next() : condTok;
1881-
const ValueType* vt = varTok->variable() ? varTok->variable()->valueType() : nullptr;
1882-
if (!(vt && vt->type == ValueType::Type::BOOL && !vt->pointer))
1883-
continue;
1884-
1885-
if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == varTok->varId()))
1886-
continue;
1887-
if (!(assignTok->astOperand2() && assignTok->astOperand2()->hasKnownIntValue()))
1888-
continue;
1889-
const MathLib::bigint val = assignTok->astOperand2()->getKnownIntValue();
1890-
if (val < 0 || val > 1)
1891-
continue;
1892-
isRedundant = (isNegation && val == 0) || (!isNegation && val == 1);
1914+
if (checkBoolConditionalAssign(condTok, assignTok, isRedundant)) {
1915+
isBoolVar = true;
18931916
} else { // comparison
18941917
if (!isSameExpression(
18951918
true, condTok->astOperand1(), assignTok->astOperand1(), mSettings, true, true))
@@ -1898,17 +1921,17 @@ void CheckConditionImpl::checkDuplicateConditionalAssign()
18981921
true, condTok->astOperand2(), assignTok->astOperand2(), mSettings, true, true))
18991922
continue;
19001923
}
1901-
duplicateConditionalAssignError(condTok, assignTok, isRedundant);
1924+
duplicateConditionalAssignError(condTok, assignTok, isRedundant, isBoolVar);
19021925
}
19031926
}
19041927
}
19051928

1906-
void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant)
1929+
void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant, bool isBoolVar)
19071930
{
19081931
ErrorPath errors;
19091932
std::string msg = "Duplicate expression for the condition and assignment.";
19101933
if (condTok && assignTok) {
1911-
if (condTok->str() == "==") {
1934+
if (condTok->str() == "==" && !isBoolVar) {
19121935
msg = "Assignment '" + assignTok->expressionString() + "' is redundant with condition '" + condTok->expressionString() + "'.";
19131936
errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "'");
19141937
errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "' is redundant");
@@ -1924,7 +1947,6 @@ void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, c
19241947
std::move(errors), Severity::style, "duplicateConditionalAssign", msg, CWE398, Certainty::normal);
19251948
}
19261949

1927-
19281950
void CheckConditionImpl::checkAssignmentInCondition()
19291951
{
19301952
if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("assignmentInCondition"))

‎lib/checkcondition.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ class CPPCHECKLIB CheckConditionImpl : public CheckImpl {
176176
void invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace);
177177
void pointerAdditionResultNotNullError(const Token *tok, const Token *calc);
178178

179-
void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant = false);
179+
void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant = false, bool isBoolVar = false);
180180

181181
void assignmentInCondition(const Token *eq);
182182

‎test/testcondition.cpp‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6406,6 +6406,50 @@ class TestCondition : public TestFixture {
64066406
"[test.cpp:7:19]: note: Assignment 'b=false'\n"
64076407
"[test.cpp:7:13]: note: Condition '!b' is redundant\n",
64086408
errout_str());
6409+
6410+
check("void f(bool& b) {\n" // #14915
6411+
" if (b == true)\n"
6412+
" b = false;\n"
6413+
"}\n"
6414+
"void g(bool& b) {\n"
6415+
" if (b == false)\n"
6416+
" b = false;\n"
6417+
"}\n"
6418+
"void h(bool& b) {\n"
6419+
" if (b != true)\n"
6420+
" b = false;\n"
6421+
"}\n"
6422+
"void i(bool& b) {\n"
6423+
" if (b != false)\n"
6424+
" b = false;\n"
6425+
"}\n"
6426+
"void j(bool& b) {\n"
6427+
" if (b == true)\n"
6428+
" b = true;\n"
6429+
"}\n"
6430+
"void k(bool& b) {\n"
6431+
" if (true == b)\n"
6432+
" b = false;\n"
6433+
"}\n");
6434+
ASSERT_EQUALS("[test.cpp:2:11]: style: The statement 'if (b==true) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n"
6435+
"[test.cpp:3:11]: note: Assignment 'b=false'\n"
6436+
"[test.cpp:2:11]: note: Condition 'b==true' is redundant\n"
6437+
"[test.cpp:6:11]: style: The statement 'if (b==false) b=false' is redundant. [duplicateConditionalAssign]\n"
6438+
"[test.cpp:7:11]: note: Assignment 'b=false'\n"
6439+
"[test.cpp:6:11]: note: Condition 'b==false' is redundant\n"
6440+
"[test.cpp:10:11]: style: The statement 'if (b!=true) b=false' is redundant. [duplicateConditionalAssign]\n"
6441+
"[test.cpp:11:11]: note: Assignment 'b=false'\n"
6442+
"[test.cpp:10:11]: note: Condition 'b!=true' is redundant\n"
6443+
"[test.cpp:14:11]: style: The statement 'if (b!=false) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n"
6444+
"[test.cpp:15:11]: note: Assignment 'b=false'\n"
6445+
"[test.cpp:14:11]: note: Condition 'b!=false' is redundant\n"
6446+
"[test.cpp:18:11]: style: The statement 'if (b==true) b=true' is redundant. [duplicateConditionalAssign]\n"
6447+
"[test.cpp:19:11]: note: Assignment 'b=true'\n"
6448+
"[test.cpp:18:11]: note: Condition 'b==true' is redundant\n"
6449+
"[test.cpp:22:14]: style: The statement 'if (true==b) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n"
6450+
"[test.cpp:23:11]: note: Assignment 'b=false'\n"
6451+
"[test.cpp:22:14]: note: Condition 'true==b' is redundant\n",
6452+
errout_str());
64096453
}
64106454

64116455
void checkAssignmentInCondition() {

0 commit comments

Comments
 (0)