diff --git a/lib/checkassert.cpp b/lib/checkassert.cpp index ac30bfb90..bf9d4369a 100644 --- a/lib/checkassert.cpp +++ b/lib/checkassert.cpp @@ -36,66 +36,50 @@ void CheckAssert::assertWithSideEffects() if (!_settings->isEnabled("warning")) return; - const Token *tok = findAssertPattern(_tokenizer->tokens()); - const Token *endTok = tok ? tok->next()->link() : nullptr; + for (const Token* tok = _tokenizer->list.front(); tok; tok = tok->next()) { + if (!Token::simpleMatch(tok, "assert (")) + continue; - while (tok && endTok) { + const Token *endTok = tok->next()->link(); for (const Token* tmp = tok->next(); tmp != endTok; tmp = tmp->next()) { - checkVariableAssignment(tmp, true); + checkVariableAssignment(tmp); - if (tmp->isName() && tmp->type() == Token::eFunction) { + if (tmp->type() == Token::eFunction) { const Function* f = tmp->function(); - if (f->argCount() == 0 && f->isConst) continue; - // functions with non-const references - else if (f->argCount() != 0) { - for (std::list::const_iterator it = f->argumentList.begin(); it != f->argumentList.end(); ++it) { - if (it->isConst() || it->isLocal()) continue; - else if (it->isReference()) { - const Token* next = it->nameToken()->next(); - bool isAssigned = checkVariableAssignment(next, false); - if (isAssigned) - sideEffectInAssertError(tmp, f->name()); + if (f->nestedIn->isClassOrStruct() && !f->isStatic && !f->isConst) + sideEffectInAssertError(tmp, f->name()); // Non-const member function called + else { + const Scope* scope = f->functionScope; + if (!scope) continue; + + for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) { + if (tok2->type() != Token::eAssignmentOp && tok2->type() != Token::eIncDecOp) continue; + + const Variable* var = tok2->previous()->variable(); + if (!var || var->isLocal() || (var->isArgument() && !var->isReference() && !var->isPointer())) + continue; // See ticket #4937. Assigning function arguments not passed by reference is ok. + if (var->isArgument() && var->isPointer() && tok2->strAt(-2) != "*") + continue; // Pointers need to be dereferenced, otherwise there is no error + + bool noReturnInScope = true; + for (const Token *rt = scope->classStart; rt != scope->classEnd; rt = rt->next()) { + if (rt->str() != "return") continue; // find all return statements + if (inSameScope(rt, tok2)) { + noReturnInScope = false; + break; + } } - } - } + if (noReturnInScope) continue; - // variables in function scope - const Scope* scope = f->functionScope; - if (!scope) continue; - - for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) { - if (tok2->type() != Token::eAssignmentOp && tok2->type() != Token::eIncDecOp) continue; - const Variable* var = tok2->previous()->variable(); - if (!var || var->isLocal()) continue; - if (var->isArgument() && - (var->isConst() || (!var->isReference() && !var->isPointer()))) - // see ticket #4937. Assigning function arguments not passed by reference is ok. - continue; - std::vector returnTokens; // find all return statements - for (const Token *rt = scope->classStart; rt != scope->classEnd; rt = rt->next()) { - if (!Token::Match(rt, "return %any%")) continue; - returnTokens.push_back(rt); - } - - bool noReturnInScope = true; - for (std::vector::iterator rt = returnTokens.begin(); rt != returnTokens.end(); ++rt) { - if (inSameScope(*rt, tok2)) { - noReturnInScope = false; - break; - } - } - if (noReturnInScope) continue; - bool isAssigned = checkVariableAssignment(tok2, false); - if (isAssigned) sideEffectInAssertError(tmp, f->name()); + break; + } } } } - - tok = findAssertPattern(endTok->next()); - endTok = tok ? tok->next()->link() : nullptr; + tok = endTok; } } //--------------------------------------------------------------------------- @@ -122,30 +106,21 @@ void CheckAssert::assignmentInAssertError(const Token *tok, const std::string& v } // checks if side effects happen on the variable prior to tmp -bool CheckAssert::checkVariableAssignment(const Token* assignTok, bool reportErr /*= true*/) +void CheckAssert::checkVariableAssignment(const Token* assignTok) { const Variable* v = assignTok->previous()->variable(); - if (!v) return false; + if (!v) return; // assignment if (assignTok->isAssignmentOp() || assignTok->type() == Token::eIncDecOp) { - if (v->isConst()) return false; + if (v->isConst()) return; - if (reportErr) // report as variable assignment error - assignmentInAssertError(assignTok, v->name()); - - return true; + assignmentInAssertError(assignTok, v->name()); } - return false; // TODO: function calls on v } -const Token* CheckAssert::findAssertPattern(const Token* start) -{ - return Token::findmatch(start, "assert ( %any%"); -} - bool CheckAssert::inSameScope(const Token* returnTok, const Token* assignTok) { // TODO: even if a return is in the same scope, the assignment might not affect it. diff --git a/lib/checkassert.h b/lib/checkassert.h index 57994155d..892b032fc 100644 --- a/lib/checkassert.h +++ b/lib/checkassert.h @@ -49,11 +49,9 @@ public: void assertWithSideEffects(); protected: - bool checkVariableAssignment(const Token* tmp, bool reportErr = true); + void checkVariableAssignment(const Token* tmp); static bool inSameScope(const Token* returnTok, const Token* assignTok); - static const Token* findAssertPattern(const Token *start); - private: void sideEffectInAssertError(const Token *tok, const std::string& functionName); void assignmentInAssertError(const Token *tok, const std::string &varname); diff --git a/test/testassert.cpp b/test/testassert.cpp index 2db2389d8..d4ce144a7 100644 --- a/test/testassert.cpp +++ b/test/testassert.cpp @@ -25,7 +25,7 @@ extern std::ostringstream errout; class TestAssert : public TestFixture { public: - TestAssert() : TestFixture("TestAsserts") {} + TestAssert() : TestFixture("TestAssert") {} private: void check( @@ -53,6 +53,7 @@ private: void run() { TEST_CASE(assignmentInAssert); TEST_CASE(functionCallInAssert); + TEST_CASE(memberFunctionCallInAssert); TEST_CASE(safeFunctionCallInAssert); } @@ -133,7 +134,40 @@ private: "void foo() {\n" " assert( !SquarePack::isRank1Or8(push2) );\n" "}\n"); - TODO_ASSERT_EQUALS("", "[test.cpp:8]: (warning) Assert statement calls a function which may have desired side effects: 'isRank1Or8'.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); + } + + void memberFunctionCallInAssert() { + check("struct SquarePack {\n" + " void Foo();\n" + "};\n" + "void foo(SquarePack s) {\n" + " assert( s.Foo(); );\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Assert statement calls a function which may have desired side effects: 'Foo'.\n", errout.str()); + + check("struct SquarePack {\n" + " void Foo() const;\n" + "};\n" + "void foo(SquarePack* s) {\n" + " assert( s->Foo(); );\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct SquarePack {\n" + " static void Foo();\n" + "};\n" + "void foo(SquarePack* s) {\n" + " assert( s->Foo(); );\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct SquarePack {\n" + "};\n" + "void foo(SquarePack* s) {\n" + " assert( s->Foo(); );\n" + "}"); + ASSERT_EQUALS("", errout.str()); } void assignmentInAssert() {