Skip to content

Commit 88785dd

Browse files
danmardanmar
committed
Refactoring the redundant assignments check
1 parent 8087cfe commit 88785dd

2 files changed

Lines changed: 40 additions & 41 deletions

File tree

‎lib/checkother.cpp‎

Lines changed: 31 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -449,8 +449,7 @@ static bool hasOperand(const Token *tok, const Token *lhs, bool cpp, const Libra
449449
return hasOperand(tok->astOperand1(), lhs, cpp, library) || hasOperand(tok->astOperand2(), lhs, cpp, library);
450450
}
451451

452-
const Token *CheckOther::checkRedundantAssignmentRecursive(const Token *assign1, const Token *startToken, const Token *endToken, bool *read) const
453-
{
452+
struct CheckOther::ScopeResult CheckOther::checkRedundantAssignmentRecursive(const Token *assign1, const Token *startToken, const Token *endToken) const {
454453
// all variable ids in assign1 LHS.
455454
std::set<unsigned int> assign1LhsVarIds;
456455
bool local = true;
@@ -468,34 +467,33 @@ const Token *CheckOther::checkRedundantAssignmentRecursive(const Token *assign1,
468467
for (const Token *tok = startToken; tok != endToken; tok = tok->next()) {
469468
if (Token::simpleMatch(tok, "try {")) {
470469
// TODO: handle try
471-
*read = true;
472-
return nullptr;
470+
return ScopeResult(ScopeResult::BAILOUT);
473471
}
474472

475473
if (tok->str() == "}" && (tok->scope()->type == Scope::eFor || tok->scope()->type == Scope::eWhile)) {
476474
// TODO: handle loops better
477-
*read = true;
478-
return nullptr;
475+
return ScopeResult(ScopeResult::BAILOUT);
476+
}
477+
478+
if (Token::simpleMatch(tok, "break ;")) {
479+
return ScopeResult(ScopeResult::BREAK);
479480
}
480481

481-
if (Token::Match(tok, "break|continue|return|throw|goto")) {
482-
// TODO: handle these better
483-
*read = true;
484-
return nullptr;
482+
if (Token::Match(tok, "continue|return|throw|goto")) {
483+
// TODO: Handle these better
484+
return ScopeResult(ScopeResult::RETURN);
485485
}
486486

487487
if (Token::simpleMatch(tok, "else {"))
488488
tok = tok->linkAt(1);
489489

490490
if (Token::simpleMatch(tok, "asm (")) {
491-
*read = true;
492-
return nullptr;
491+
return ScopeResult(ScopeResult::BAILOUT);
493492
}
494493

495494
if (!local && Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) {
496495
// TODO: this is a quick bailout
497-
*read = true;
498-
return nullptr;
496+
return ScopeResult(ScopeResult::BAILOUT);
499497
}
500498

501499
if (assign1LhsVarIds.find(tok->varId()) != assign1LhsVarIds.end()) {
@@ -505,43 +503,39 @@ const Token *CheckOther::checkRedundantAssignmentRecursive(const Token *assign1,
505503
if (Token::simpleMatch(parent->astParent(), "=") && parent == parent->astParent()->astOperand1()) {
506504
if (!local && hasFunctionCall(parent->astParent()->astOperand2())) {
507505
// TODO: this is a quick bailout
508-
*read = true;
509-
return nullptr;
506+
return ScopeResult(ScopeResult::BAILOUT);
510507
}
511508
if (hasOperand(parent->astParent()->astOperand2(), assign1->astOperand1(), mTokenizer->isCPP(), mSettings->library)) {
512-
*read = true;
513-
return nullptr;
509+
return ScopeResult(ScopeResult::READ);
514510
}
515511
const bool reassign = isSameExpression(mTokenizer->isCPP(), false, assign1->astOperand1(), parent, mSettings->library, false, false, nullptr);
516512
if (reassign)
517-
return parent->astParent();
518-
*read = true;
519-
return nullptr;
513+
return ScopeResult(ScopeResult::WRITE, parent->astParent());
514+
return ScopeResult(ScopeResult::READ);
520515
} else {
521516
// TODO: this is a quick bailout
522-
*read = true;
523-
return nullptr;
517+
return ScopeResult(ScopeResult::BAILOUT);
524518
}
525519
}
526520

527521
if (Token::Match(tok, ") {")) {
528-
const Token *a1 = checkRedundantAssignmentRecursive(assign1, tok->tokAt(2), tok->linkAt(1), read);
529-
if (*read)
530-
return nullptr;
522+
const ScopeResult &result1 = checkRedundantAssignmentRecursive(assign1, tok->tokAt(2), tok->linkAt(1));
523+
if (result1.type == ScopeResult::READ || result1.type == ScopeResult::BAILOUT)
524+
return result1;
531525
if (Token::simpleMatch(tok->linkAt(1), "} else {")) {
532526
const Token *elseStart = tok->linkAt(1)->tokAt(2);
533-
const Token *a2 = checkRedundantAssignmentRecursive(assign1, elseStart, elseStart->link(), read);
534-
if (*read)
535-
return nullptr;
536-
if (a1 && a2)
537-
return a1;
527+
const ScopeResult &result2 = checkRedundantAssignmentRecursive(assign1, elseStart, elseStart->link());
528+
if (result2.type == ScopeResult::READ || result2.type == ScopeResult::BAILOUT)
529+
return result2;
530+
if (result1.type == ScopeResult::WRITE && result2.type == ScopeResult::WRITE)
531+
return result1;
538532
tok = elseStart->link();
539533
} else {
540534
tok = tok->linkAt(1);
541535
}
542536
}
543537
}
544-
return nullptr;
538+
return ScopeResult(ScopeResult::NONE);
545539
}
546540

547541
void CheckOther::checkRedundantAssignment()
@@ -610,19 +604,18 @@ void CheckOther::checkRedundantAssignment()
610604
continue;
611605

612606
// Is there a redundant assignment?
613-
bool read = false;
614607
const Token *start;
615608
if (tok->isAssignmentOp())
616609
start = tok->next();
617610
else
618611
start = tok->findExpressionStartEndTokens().second->next();
619-
const Token *nextAssign = checkRedundantAssignmentRecursive(tok, start, scope->bodyEnd, &read);
620-
if (read || !nextAssign)
612+
const ScopeResult &nextAssign = checkRedundantAssignmentRecursive(tok, start, scope->bodyEnd);
613+
if (nextAssign.type != ScopeResult::WRITE || !nextAssign.token)
621614
continue;
622615

623616
// there is redundant assignment. Is there a case between the assignments?
624617
bool hasCase = false;
625-
for (const Token *tok2 = tok; tok2 != nextAssign; tok2 = tok2->next()) {
618+
for (const Token *tok2 = tok; tok2 != nextAssign.token; tok2 = tok2->next()) {
626619
if (tok2->str() == "case") {
627620
hasCase = true;
628621
break;
@@ -631,9 +624,9 @@ void CheckOther::checkRedundantAssignment()
631624

632625
// warn
633626
if (hasCase)
634-
redundantAssignmentInSwitchError(tok, nextAssign, tok->astOperand1()->expressionString());
627+
redundantAssignmentInSwitchError(tok, nextAssign.token, tok->astOperand1()->expressionString());
635628
else
636-
redundantAssignmentError(tok, nextAssign, tok->astOperand1()->expressionString(), inconclusive);
629+
redundantAssignmentError(tok, nextAssign.token, tok->astOperand1()->expressionString(), inconclusive);
637630
}
638631
}
639632
}

‎lib/checkother.h‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,9 +216,15 @@ class CPPCHECKLIB CheckOther : public Check {
216216
void checkShadowVariables();
217217

218218
private:
219-
// Recursively check for redundant assignments..
220-
// TODO: when we support C++17, return std::variant
221-
const Token *checkRedundantAssignmentRecursive(const Token *assign1, const Token *startToken, const Token *endToken, bool *read) const;
219+
220+
struct ScopeResult {
221+
enum Type { NONE, READ, WRITE, BREAK, RETURN, BAILOUT } type;
222+
ScopeResult(Type type) : type(type), token(nullptr) {}
223+
ScopeResult(Type type, const Token *token) : type(type), token(token) {}
224+
const Token *token;
225+
};
226+
/** Recursively check for redundant assignments. */
227+
struct ScopeResult checkRedundantAssignmentRecursive(const Token *assign1, const Token *startToken, const Token *endToken) const;
222228

223229
// Error messages..
224230
void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result);

0 commit comments

Comments
 (0)