diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 231a04904..de1184368 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1037,3 +1037,135 @@ bool isLikelyStreamRead(bool cpp, const Token *op) return (!parent->astOperand1()->valueType() || !parent->astOperand1()->valueType()->isIntegral()); } + +static bool nonLocal(const Variable* var) +{ + return !var || (!var->isLocal() && !var->isArgument()) || var->isStatic() || var->isReference(); +} + +static bool hasFunctionCall(const Token *tok) +{ + if (!tok) + return false; + if (Token::Match(tok, "%name% (")) + // todo, const/pure function? + return true; + return hasFunctionCall(tok->astOperand1()) || hasFunctionCall(tok->astOperand2()); +} + +FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *assign1, const Token *startToken, const Token *endToken) +{ + // all variable ids in assign1 LHS. + std::set assign1LhsVarIds; + bool local = true; + visitAstNodes(assign1->astOperand1(), + [&](const Token *tok) { + if (tok->varId() > 0) { + assign1LhsVarIds.insert(tok->varId()); + if (!Token::simpleMatch(tok->previous(), ".")) + local &= !nonLocal(tok->variable()); + } + return ChildrenToVisit::op1_and_op2; + }); + + // Parse the given tokens + for (const Token *tok = startToken; tok != endToken; tok = tok->next()) { + if (Token::simpleMatch(tok, "try {")) { + // TODO: handle try + return Result(Result::Type::BAILOUT); + } + + if (tok->str() == "}" && (tok->scope()->type == Scope::eFor || tok->scope()->type == Scope::eWhile)) { + // TODO: handle loops better + return Result(Result::Type::BAILOUT); + } + + if (Token::simpleMatch(tok, "break ;")) { + return Result(Result::Type::BREAK, tok); + } + + if (Token::Match(tok, "continue|return|throw|goto")) { + // TODO: Handle these better + return Result(Result::Type::RETURN); + } + + if (Token::simpleMatch(tok, "else {")) + tok = tok->linkAt(1); + + if (Token::simpleMatch(tok, "asm (")) { + return Result(Result::Type::BAILOUT); + } + + if (!local && Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { + // TODO: this is a quick bailout + return Result(Result::Type::BAILOUT); + } + + if (assign1LhsVarIds.find(tok->varId()) != assign1LhsVarIds.end()) { + const Token *parent = tok; + while (Token::Match(parent->astParent(), ".|::|[")) + parent = parent->astParent(); + if (Token::simpleMatch(parent->astParent(), "=") && parent == parent->astParent()->astOperand1()) { + if (!local && hasFunctionCall(parent->astParent()->astOperand2())) { + // TODO: this is a quick bailout + return Result(Result::Type::BAILOUT); + } + if (hasOperand(parent->astParent()->astOperand2(), assign1->astOperand1())) { + return Result(Result::Type::READ); + } + const bool reassign = isSameExpression(mCpp, false, assign1->astOperand1(), parent, mLibrary, false, false, nullptr); + if (reassign) + return Result(Result::Type::WRITE, parent->astParent()); + return Result(Result::Type::READ); + } else { + // TODO: this is a quick bailout + return Result(Result::Type::BAILOUT); + } + } + + if (Token::Match(tok, ") {")) { + const Result &result1 = checkRecursive(assign1, tok->tokAt(2), tok->linkAt(1)); + if (result1.type == Result::Type::READ || result1.type == Result::Type::BAILOUT) + return result1; + if (Token::simpleMatch(tok->linkAt(1), "} else {")) { + const Token *elseStart = tok->linkAt(1)->tokAt(2); + const Result &result2 = checkRecursive(assign1, elseStart, elseStart->link()); + if (result2.type == Result::Type::READ || result2.type == Result::Type::BAILOUT) + return result2; + if (result1.type == Result::Type::WRITE && result2.type == Result::Type::WRITE) + return result1; + tok = elseStart->link(); + } else { + tok = tok->linkAt(1); + } + } + } + + return Result(Result::Type::NONE); +} + +FwdAnalysis::Result FwdAnalysis::check(const Token *assign1, const Token *startToken, const Token *endToken) +{ + Result result = checkRecursive(assign1, startToken, endToken); + + // Break => continue checking in outer scope + while (result.type == FwdAnalysis::Result::Type::BREAK) { + const Scope *s = result.token->scope(); + while (s->type == Scope::eIf) + s = s->nestedIn; + if (s->type != Scope::eSwitch) + break; + result = checkRecursive(assign1, s->bodyEnd->next(), endToken); + } + + return result; +} + +bool FwdAnalysis::hasOperand(const Token *tok, const Token *lhs) const +{ + if (!tok) + return false; + if (isSameExpression(mCpp, false, tok, lhs, mLibrary, false, false, nullptr)) + return true; + return hasOperand(tok->astOperand1(), lhs) || hasOperand(tok->astOperand2(), lhs); +} diff --git a/lib/astutils.h b/lib/astutils.h index e7a7e423e..83c96f76b 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -160,4 +160,28 @@ const Token *findLambdaEndToken(const Token *first); */ bool isLikelyStreamRead(bool cpp, const Token *op); +class FwdAnalysis { +public: + FwdAnalysis(bool cpp, const Library &library) : mCpp(cpp), mLibrary(library) {} + + /** Result of forward analysis */ + struct Result { + enum class Type { NONE, READ, WRITE, BREAK, RETURN, BAILOUT } type; + Result(Type type) : type(type), token(nullptr) {} + Result(Type type, const Token *token) : type(type), token(token) {} + const Token *token; + }; + + /** forward analysis */ + struct Result check(const Token *assign1, const Token *startToken, const Token *endToken); + + bool hasOperand(const Token *tok, const Token *lhs) const; + +private: + struct Result checkRecursive(const Token *assign1, const Token *startToken, const Token *endToken); + + const bool mCpp; + const Library &mLibrary; +}; + #endif // astutilsH diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 048d94b44..1971b3564 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -412,132 +412,6 @@ void CheckOther::checkPipeParameterSizeError(const Token *tok, const std::string // Detect redundant assignments: x = 0; x = 4; //--------------------------------------------------------------------------- -static bool nonLocal(const Variable* var) -{ - return !var || (!var->isLocal() && !var->isArgument()) || var->isStatic() || var->isReference(); -} - -static void eraseMemberAssignments(const unsigned int varId, const std::map > &membervars, std::map &varAssignments) -{ - const std::map >::const_iterator it = membervars.find(varId); - if (it != membervars.end()) { - const std::set& vars = it->second; - for (unsigned int var : vars) { - varAssignments.erase(var); - if (var != varId) - eraseMemberAssignments(var, membervars, varAssignments); - } - } -} - -static bool hasFunctionCall(const Token *tok) -{ - if (!tok) - return false; - if (Token::Match(tok, "%name% (")) - // todo, const/pure function? - return true; - return hasFunctionCall(tok->astOperand1()) || hasFunctionCall(tok->astOperand2()); -} - -static bool hasOperand(const Token *tok, const Token *lhs, bool cpp, const Library &library) -{ - if (!tok) - return false; - if (isSameExpression(cpp, false, tok, lhs, library, false, false, nullptr)) - return true; - return hasOperand(tok->astOperand1(), lhs, cpp, library) || hasOperand(tok->astOperand2(), lhs, cpp, library); -} - -struct CheckOther::ScopeResult CheckOther::checkRedundantAssignmentRecursive(const Token *assign1, const Token *startToken, const Token *endToken) const { - // all variable ids in assign1 LHS. - std::set assign1LhsVarIds; - bool local = true; - visitAstNodes(assign1->astOperand1(), - [&](const Token *tok) { - if (tok->varId() > 0) { - assign1LhsVarIds.insert(tok->varId()); - if (!Token::simpleMatch(tok->previous(), ".")) - local &= !nonLocal(tok->variable()); - } - return ChildrenToVisit::op1_and_op2; - }); - - // Parse the given tokens - for (const Token *tok = startToken; tok != endToken; tok = tok->next()) { - if (Token::simpleMatch(tok, "try {")) { - // TODO: handle try - return ScopeResult(ScopeResult::BAILOUT); - } - - if (tok->str() == "}" && (tok->scope()->type == Scope::eFor || tok->scope()->type == Scope::eWhile)) { - // TODO: handle loops better - return ScopeResult(ScopeResult::BAILOUT); - } - - if (Token::simpleMatch(tok, "break ;")) { - return ScopeResult(ScopeResult::BREAK, tok); - } - - if (Token::Match(tok, "continue|return|throw|goto")) { - // TODO: Handle these better - return ScopeResult(ScopeResult::RETURN); - } - - if (Token::simpleMatch(tok, "else {")) - tok = tok->linkAt(1); - - if (Token::simpleMatch(tok, "asm (")) { - return ScopeResult(ScopeResult::BAILOUT); - } - - if (!local && Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { - // TODO: this is a quick bailout - return ScopeResult(ScopeResult::BAILOUT); - } - - if (assign1LhsVarIds.find(tok->varId()) != assign1LhsVarIds.end()) { - const Token *parent = tok; - while (Token::Match(parent->astParent(), ".|::|[")) - parent = parent->astParent(); - if (Token::simpleMatch(parent->astParent(), "=") && parent == parent->astParent()->astOperand1()) { - if (!local && hasFunctionCall(parent->astParent()->astOperand2())) { - // TODO: this is a quick bailout - return ScopeResult(ScopeResult::BAILOUT); - } - if (hasOperand(parent->astParent()->astOperand2(), assign1->astOperand1(), mTokenizer->isCPP(), mSettings->library)) { - return ScopeResult(ScopeResult::READ); - } - const bool reassign = isSameExpression(mTokenizer->isCPP(), false, assign1->astOperand1(), parent, mSettings->library, false, false, nullptr); - if (reassign) - return ScopeResult(ScopeResult::WRITE, parent->astParent()); - return ScopeResult(ScopeResult::READ); - } else { - // TODO: this is a quick bailout - return ScopeResult(ScopeResult::BAILOUT); - } - } - - if (Token::Match(tok, ") {")) { - const ScopeResult &result1 = checkRedundantAssignmentRecursive(assign1, tok->tokAt(2), tok->linkAt(1)); - if (result1.type == ScopeResult::READ || result1.type == ScopeResult::BAILOUT) - return result1; - if (Token::simpleMatch(tok->linkAt(1), "} else {")) { - const Token *elseStart = tok->linkAt(1)->tokAt(2); - const ScopeResult &result2 = checkRedundantAssignmentRecursive(assign1, elseStart, elseStart->link()); - if (result2.type == ScopeResult::READ || result2.type == ScopeResult::BAILOUT) - return result2; - if (result1.type == ScopeResult::WRITE && result2.type == ScopeResult::WRITE) - return result1; - tok = elseStart->link(); - } else { - tok = tok->linkAt(1); - } - } - } - return ScopeResult(ScopeResult::NONE); -} - void CheckOther::checkRedundantAssignment() { if (!mSettings->isEnabled(Settings::STYLE)) @@ -586,9 +460,6 @@ void CheckOther::checkRedundantAssignment() // todo: check static variables continue; - if (hasOperand(tok->astOperand2(), tok->astOperand1(), mTokenizer->isCPP(), mSettings->library)) - continue; - // If there is a custom assignment operator => this is inconclusive bool inconclusive = false; if (mTokenizer->isCPP() && tok->astOperand1()->valueType() && tok->astOperand1()->valueType()->typeScope) { @@ -603,6 +474,10 @@ void CheckOther::checkRedundantAssignment() if (inconclusive && !mSettings->inconclusive) continue; + FwdAnalysis fwdAnalysis(mTokenizer->isCPP(), mSettings->library); + if (fwdAnalysis.hasOperand(tok->astOperand2(), tok->astOperand1())) + continue; + // Is there a redundant assignment? const Token *start; if (tok->isAssignmentOp()) @@ -611,25 +486,9 @@ void CheckOther::checkRedundantAssignment() start = tok->findExpressionStartEndTokens().second->next(); // Get next assignment.. - ScopeResult nextAssign(ScopeResult::NONE); - while (true) { - nextAssign = checkRedundantAssignmentRecursive(tok, start, scope->bodyEnd); + FwdAnalysis::Result nextAssign = fwdAnalysis.check(tok, start, scope->bodyEnd); - // Break => continue checking in outer scope - if (nextAssign.type == ScopeResult::BREAK) { - const Scope *s = nextAssign.token->scope(); - while (s->type == Scope::eIf) - s = s->nestedIn; - if (s->type == Scope::eSwitch) { - start = s->bodyEnd->next(); - continue; - } - } - - break; - } - - if (nextAssign.type != ScopeResult::WRITE || !nextAssign.token) + if (nextAssign.type != FwdAnalysis::Result::Type::WRITE || !nextAssign.token) continue; // there is redundant assignment. Is there a case between the assignments? diff --git a/lib/checkother.h b/lib/checkother.h index 5a184b0e4..f4600f78c 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -217,15 +217,6 @@ public: private: - struct ScopeResult { - enum Type { NONE, READ, WRITE, BREAK, RETURN, BAILOUT } type; - ScopeResult(Type type) : type(type), token(nullptr) {} - ScopeResult(Type type, const Token *token) : type(type), token(token) {} - const Token *token; - }; - /** Recursively check for redundant assignments. */ - struct ScopeResult checkRedundantAssignmentRecursive(const Token *assign1, const Token *startToken, const Token *endToken) const; - // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result); void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName);