Skip to content

Fix #14915 FN duplicateConditionalAssign with bool and explicit check #8827

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chrchr-github wants to merge 4 commits into
base: main
Choose a base branch
from
+87 −21
Open

Fix #14915 FN duplicateConditionalAssign with bool and explicit check #8827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8efb303
Update checkcondition.h
chrchr-github Sep 4, 2026
5b24e80
Update checkcondition.cpp
chrchr-github Sep 4, 2026
9c6fbd8
Update testcondition.cpp
chrchr-github Sep 4, 2026
cc00b2d
Update checkcondition.cpp
chrchr-github Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 42 additions & 20 deletions lib/checkcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1846,6 +1846,42 @@ void CheckConditionImpl::pointerAdditionResultNotNullError(const Token *tok, con
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.");
}

static bool checkBoolConditionalAssign(const Token* condTok, const Token* assignTok, bool& isRedundant)
{
bool isNegation = false;
const Token* varTok = condTok;
if (condTok->isUnaryOp("!")) {
isNegation = true;
varTok = varTok->astOperand1();
} else if (condTok->isBinaryOp()) {
varTok = condTok->astOperand1();
if (varTok->hasKnownIntValue())
varTok = condTok->astOperand2();
}

const ValueType* vt = varTok->variable() ? varTok->variable()->valueType() : nullptr;
if (!(vt && vt->type == ValueType::Type::BOOL && !vt->pointer))
return false;

if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == varTok->varId()))
return false;
if (!(assignTok->astOperand2() && assignTok->astOperand2()->hasKnownIntValue()))
return false;
const MathLib::bigint val = assignTok->astOperand2()->getKnownIntValue();
if (val < 0 || val > 1)
return false;
if (condTok->isBinaryOp()) {
if (!varTok->astSibling()->hasKnownIntValue())
return false;
const MathLib::bigint compVal = varTok->astSibling()->getKnownIntValue();
if (compVal < 0 || compVal > 1)
return false;
isNegation = (condTok->str() == "!=") == (compVal == 1);
}
isRedundant = (isNegation && val == 0) || (!isNegation && val == 1);
return true;
}

void CheckConditionImpl::checkDuplicateConditionalAssign()
{
if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("duplicateConditionalAssign"))
Expand All @@ -1862,7 +1898,7 @@ void CheckConditionImpl::checkDuplicateConditionalAssign()
continue;
const Token *blockTok = tok->linkAt(1)->next();
const Token *condTok = tok->next()->astOperand2();
const bool isBoolVar = Token::Match(condTok, "!| %var%");
bool isBoolVar = Token::Match(condTok, "!| %var%");
if (!isBoolVar && !Token::Match(condTok, "==|!="))
continue;
if ((isBoolVar || condTok->str() == "!=") && Token::simpleMatch(blockTok->link(), "} else {"))
Expand All @@ -1875,21 +1911,8 @@ void CheckConditionImpl::checkDuplicateConditionalAssign()
if (nextAfterAstRightmostLeaf(assignTok) != blockTok->link()->previous())
continue;
bool isRedundant = false;
if (isBoolVar) {
const bool isNegation = condTok->str() == "!";
const Token* const varTok = isNegation ? condTok->next() : condTok;
const ValueType* vt = varTok->variable() ? varTok->variable()->valueType() : nullptr;
if (!(vt && vt->type == ValueType::Type::BOOL && !vt->pointer))
continue;

if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == varTok->varId()))
continue;
if (!(assignTok->astOperand2() && assignTok->astOperand2()->hasKnownIntValue()))
continue;
const MathLib::bigint val = assignTok->astOperand2()->getKnownIntValue();
if (val < 0 || val > 1)
continue;
isRedundant = (isNegation && val == 0) || (!isNegation && val == 1);
if (checkBoolConditionalAssign(condTok, assignTok, isRedundant)) {
isBoolVar = true;
} else { // comparison
if (!isSameExpression(
true, condTok->astOperand1(), assignTok->astOperand1(), mSettings, true, true))
Expand All @@ -1898,17 +1921,17 @@ void CheckConditionImpl::checkDuplicateConditionalAssign()
true, condTok->astOperand2(), assignTok->astOperand2(), mSettings, true, true))
continue;
}
duplicateConditionalAssignError(condTok, assignTok, isRedundant);
duplicateConditionalAssignError(condTok, assignTok, isRedundant, isBoolVar);
}
}
}

void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant)
void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant, bool isBoolVar)
{
ErrorPath errors;
std::string msg = "Duplicate expression for the condition and assignment.";
if (condTok && assignTok) {
if (condTok->str() == "==") {
if (condTok->str() == "==" && !isBoolVar) {
msg = "Assignment '" + assignTok->expressionString() + "' is redundant with condition '" + condTok->expressionString() + "'.";
errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "'");
errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "' is redundant");
Expand All @@ -1924,7 +1947,6 @@ void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, c
std::move(errors), Severity::style, "duplicateConditionalAssign", msg, CWE398, Certainty::normal);
}


void CheckConditionImpl::checkAssignmentInCondition()
{
if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("assignmentInCondition"))
Expand Down
2 changes: 1 addition & 1 deletion lib/checkcondition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ class CPPCHECKLIB CheckConditionImpl : public CheckImpl {
void invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace);
void pointerAdditionResultNotNullError(const Token *tok, const Token *calc);

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

void assignmentInCondition(const Token *eq);

Expand Down
44 changes: 44 additions & 0 deletions test/testcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6406,6 +6406,50 @@ class TestCondition : public TestFixture {
"[test.cpp:7:19]: note: Assignment 'b=false'\n"
"[test.cpp:7:13]: note: Condition '!b' is redundant\n",
errout_str());

check("void f(bool& b) {\n" // #14915
" if (b == true)\n"
" b = false;\n"
"}\n"
"void g(bool& b) {\n"
" if (b == false)\n"
" b = false;\n"
"}\n"
"void h(bool& b) {\n"
" if (b != true)\n"
" b = false;\n"
"}\n"
"void i(bool& b) {\n"
" if (b != false)\n"
" b = false;\n"
"}\n"
"void j(bool& b) {\n"
" if (b == true)\n"
" b = true;\n"
"}\n"
"void k(bool& b) {\n"
" if (true == b)\n"
" b = false;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2:11]: style: The statement 'if (b==true) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n"
"[test.cpp:3:11]: note: Assignment 'b=false'\n"
"[test.cpp:2:11]: note: Condition 'b==true' is redundant\n"
"[test.cpp:6:11]: style: The statement 'if (b==false) b=false' is redundant. [duplicateConditionalAssign]\n"
"[test.cpp:7:11]: note: Assignment 'b=false'\n"
"[test.cpp:6:11]: note: Condition 'b==false' is redundant\n"
"[test.cpp:10:11]: style: The statement 'if (b!=true) b=false' is redundant. [duplicateConditionalAssign]\n"
"[test.cpp:11:11]: note: Assignment 'b=false'\n"
"[test.cpp:10:11]: note: Condition 'b!=true' is redundant\n"
"[test.cpp:14:11]: style: The statement 'if (b!=false) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n"
"[test.cpp:15:11]: note: Assignment 'b=false'\n"
"[test.cpp:14:11]: note: Condition 'b!=false' is redundant\n"
"[test.cpp:18:11]: style: The statement 'if (b==true) b=true' is redundant. [duplicateConditionalAssign]\n"
"[test.cpp:19:11]: note: Assignment 'b=true'\n"
"[test.cpp:18:11]: note: Condition 'b==true' is redundant\n"
"[test.cpp:22:14]: style: The statement 'if (true==b) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n"
"[test.cpp:23:11]: note: Assignment 'b=false'\n"
"[test.cpp:22:14]: note: Condition 'true==b' is redundant\n",
errout_str());
}

void checkAssignmentInCondition() {
Expand Down
Loading