diff --git a/Makefile b/Makefile index e268b4498..08ddb56e1 100644 --- a/Makefile +++ b/Makefile @@ -93,6 +93,7 @@ MAN_SOURCE=man/cppcheck.1.xml ###### Object Files LIBOBJ = $(SRCDIR)/check64bit.o \ + $(SRCDIR)/checkassert.o \ $(SRCDIR)/checkassignif.o \ $(SRCDIR)/checkautovariables.o \ $(SRCDIR)/checkbool.o \ @@ -138,6 +139,7 @@ CLIOBJ = cli/cmdlineparser.o \ TESTOBJ = test/options.o \ test/test64bit.o \ + test/testassert.o \ test/testassignif.o \ test/testautovariables.o \ test/testbool.o \ @@ -234,6 +236,9 @@ install: cppcheck $(SRCDIR)/check64bit.o: lib/check64bit.cpp lib/check64bit.h lib/config.h lib/check.h lib/token.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/standards.h lib/symboldatabase.h lib/mathlib.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/check64bit.o $(SRCDIR)/check64bit.cpp +$(SRCDIR)/checkassert.o: lib/checkassert.cpp lib/checkassert.h lib/config.h lib/check.h lib/token.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/standards.h lib/symboldatabase.h lib/mathlib.h + $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/checkassert.o $(SRCDIR)/checkassert.cpp + $(SRCDIR)/checkassignif.o: lib/checkassignif.cpp lib/checkassignif.h lib/config.h lib/check.h lib/token.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/standards.h lib/mathlib.h lib/symboldatabase.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/checkassignif.o $(SRCDIR)/checkassignif.cpp @@ -363,6 +368,9 @@ test/options.o: test/options.cpp test/options.h test/test64bit.o: test/test64bit.cpp lib/tokenize.h lib/errorlogger.h lib/config.h lib/suppressions.h lib/tokenlist.h lib/check64bit.h lib/check.h lib/token.h lib/settings.h lib/standards.h test/testsuite.h test/redirect.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/test64bit.o test/test64bit.cpp +test/testassert.o: test/testassert.cpp lib/tokenize.h lib/errorlogger.h lib/config.h lib/suppressions.h lib/tokenlist.h lib/checkassert.h lib/check.h lib/token.h lib/settings.h lib/standards.h test/testsuite.h test/redirect.h + $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testassert.o test/testassert.cpp + test/testassignif.o: test/testassignif.cpp lib/tokenize.h lib/errorlogger.h lib/config.h lib/suppressions.h lib/tokenlist.h lib/checkassignif.h lib/check.h lib/token.h lib/settings.h lib/standards.h lib/mathlib.h test/testsuite.h test/redirect.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testassignif.o test/testassignif.cpp diff --git a/lib/checkassert.cpp b/lib/checkassert.cpp new file mode 100644 index 000000000..2324567c3 --- /dev/null +++ b/lib/checkassert.cpp @@ -0,0 +1,147 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +//--------------------------------------------------------------------------- +// You should not write statements with side effects in assert() +//--------------------------------------------------------------------------- + +#include "checkassert.h" +#include "symboldatabase.h" + +//--------------------------------------------------------------------------- + + +// Register this check class (by creating a static instance of it) +namespace { + CheckAssert instance; +} + +void CheckAssert::assertWithSideEffects() +{ + if (!_settings->isEnabled("warning")) + return; + + const Token *tok = findAssertPattern(_tokenizer->tokens()); + const Token *endTok = tok ? tok->next()->link() : NULL; + + while (tok && endTok) { + for (const Token* tmp = tok->tokAt(1); tmp != endTok; tmp = tmp->next()) { + checkVariableAssignment(tmp, true); + + if (tmp->isName() && 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()); + 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 && tok2->type() != Token::eAssignmentOp && tok2->type() != Token::eIncDecOp) continue; + const Variable* var = tok2->previous()->variable(); + if (!var) 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) { + noReturnInScope &= !inSameScope(*rt, tok2); + } + if (noReturnInScope) continue; + bool isAssigned = checkVariableAssignment(tok2, false); + if (isAssigned) + sideEffectInAssertError(tmp, f->name()); + } + } + } + + tok = findAssertPattern(endTok->next()); + endTok = tok ? tok->next()->link() : NULL; + } +} +//--------------------------------------------------------------------------- + + +void CheckAssert::sideEffectInAssertError(const Token *tok, const std::string& functionName) +{ + reportError(tok, Severity::warning, + "assertWithSideEffect", "Assert statement calls a function which may have desired side effects: '" + functionName + "'.\n" + "Non-pure function: '" + functionName + "' is called inside assert statement. " + "Assert statements are removed from release builds so the code inside " + "assert statement is not executed. If the code is needed also in release " + "builds, this is a bug."); +} + +void CheckAssert::assignmentInAssertError(const Token *tok, const std::string& varname) +{ + reportError(tok, Severity::warning, + "assignmentInAssert", "Assert statement modifies '" + varname + "'.\n" + "Variable '" + varname + "' is modified insert assert statement. " + "Assert statements are removed from release builds so the code inside " + "assert statement is not executed. If the code is needed also in release " + "builds, this is a bug."); +} + +// checks if side effects happen on the variable prior to tmp +bool CheckAssert::checkVariableAssignment(const Token* assignTok, bool reportError /*= true*/) +{ + const Variable* v = assignTok->previous()->variable(); + if (!v) return false; + + // assignment + if (assignTok->isAssignmentOp() || assignTok->type() == Token::eIncDecOp) { + + if (v->isConst()) return false; + + if (reportError) // report as variable assignment error + assignmentInAssertError(assignTok, v->name()); + + return true; + } + 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. + return returnTok->scope() == assignTok->scope(); +} diff --git a/lib/checkassert.h b/lib/checkassert.h new file mode 100644 index 000000000..030770d9f --- /dev/null +++ b/lib/checkassert.h @@ -0,0 +1,76 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + + +//--------------------------------------------------------------------------- +#ifndef CheckAssertH +#define CheckAssertH +//--------------------------------------------------------------------------- + +#include "config.h" +#include "check.h" + +/// @addtogroup Checks +/// @{ + +/** + * @brief Checking for side effects in assert statements + */ + +class CPPCHECKLIB CheckAssert : public Check { +public: + CheckAssert() : Check(myName()) + {} + + CheckAssert(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) + : Check(myName(), tokenizer, settings, errorLogger) + {} + + virtual void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { + CheckAssert check(tokenizer, settings, errorLogger); + check.assertWithSideEffects(); + } + + void assertWithSideEffects(); + +protected: + bool checkVariableAssignment(const Token* tmp, bool reportError = true); + 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); + + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { + CheckAssert c(0, settings, errorLogger); + c.assertWithSideEffects(); + } + + static std::string myName() { + return "Side Effects in asserts"; + } + + std::string classInfo() const { + return "Warn if side effects in assert statements \n"; + } +}; +/// @} +//--------------------------------------------------------------------------- +#endif \ No newline at end of file diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 299ed0c68..3d3e2022d 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1170,46 +1170,6 @@ void CheckOther::selfAssignmentError(const Token *tok, const std::string &varnam "selfAssignment", "Redundant assignment of '" + varname + "' to itself."); } -//--------------------------------------------------------------------------- -// int a = 1; -// assert(a = 2); // <- assert should not have a side-effect -//--------------------------------------------------------------------------- -static inline const Token *findAssertPattern(const Token *start) -{ - return Token::findmatch(start, "assert ( %any%"); -} - -void CheckOther::checkAssignmentInAssert() -{ - if (!_settings->isEnabled("warning")) - return; - - const Token *tok = findAssertPattern(_tokenizer->tokens()); - const Token *endTok = tok ? tok->next()->link() : NULL; - - while (tok && endTok) { - for (tok = tok->tokAt(2); tok != endTok; tok = tok->next()) { - if (tok->isName() && (tok->next()->isAssignmentOp() || tok->next()->type() == Token::eIncDecOp)) - assignmentInAssertError(tok, tok->str()); - else if (Token::Match(tok, "--|++ %var%")) - assignmentInAssertError(tok, tok->strAt(1)); - } - - tok = findAssertPattern(endTok->next()); - endTok = tok ? tok->next()->link() : NULL; - } -} - -void CheckOther::assignmentInAssertError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::warning, - "assignmentInAssert", "Assert statement modifies '" + varname + "'.\n" - "Variable '" + varname + "' is modified insert assert statement. " - "Assert statements are removed from release builds so the code inside " - "assert statement is not executed. If the code is needed also in release " - "builds, this is a bug."); -} - //--------------------------------------------------------------------------- // if ((x != 1) || (x != 3)) // expression always true // if ((x == 1) && (x == 3)) // expression always false diff --git a/lib/checkother.h b/lib/checkother.h index 248037ce1..7b5b0a3c4 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -60,7 +60,6 @@ public: checkOther.checkRedundantAssignment(); checkOther.checkRedundantAssignmentInSwitch(); checkOther.checkSuspiciousCaseInSwitch(); - checkOther.checkAssignmentInAssert(); checkOther.checkSelfAssignment(); checkOther.checkDuplicateIf(); checkOther.checkDuplicateBranch(); @@ -186,9 +185,6 @@ public: /** @brief %Check for assigning a variable to itself*/ void checkSelfAssignment(); - /** @brief %Check for assignment to a variable in an assert test*/ - void checkAssignmentInAssert(); - /** @brief %Check for testing for mutual exclusion over ||*/ void checkIncorrectLogicOperator(); @@ -293,7 +289,6 @@ private: void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString); void suspiciousEqualityComparisonError(const Token* tok); void selfAssignmentError(const Token *tok, const std::string &varname); - void assignmentInAssertError(const Token *tok, const std::string &varname); void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always); void redundantConditionError(const Token *tok, const std::string &text); void misusedScopeObjectError(const Token *tok, const std::string &varname); @@ -357,7 +352,6 @@ private: c.suspiciousCaseInSwitchError(0, "||"); c.suspiciousEqualityComparisonError(0); c.selfAssignmentError(0, "varname"); - c.assignmentInAssertError(0, "varname"); c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true); c.redundantConditionError(0, "If x > 10 the condition x > 11 is always true."); c.memsetZeroBytesError(0, "varname"); diff --git a/lib/cppcheck.vcxproj b/lib/cppcheck.vcxproj index 131110c3d..a98131f53 100644 --- a/lib/cppcheck.vcxproj +++ b/lib/cppcheck.vcxproj @@ -36,6 +36,7 @@ + @@ -75,6 +76,7 @@ + diff --git a/lib/cppcheck.vcxproj.filters b/lib/cppcheck.vcxproj.filters index f075b3fb2..f43bd271a 100644 --- a/lib/cppcheck.vcxproj.filters +++ b/lib/cppcheck.vcxproj.filters @@ -119,6 +119,9 @@ Source Files + + Source Files + @@ -238,6 +241,9 @@ Header Files + + Header Files + diff --git a/lib/lib.pri b/lib/lib.pri index f8c34815f..f395f0935 100644 --- a/lib/lib.pri +++ b/lib/lib.pri @@ -4,6 +4,7 @@ include($$PWD/pcrerules.pri) INCLUDEPATH += ../externals/tinyxml HEADERS += $${BASEPATH}check.h \ $${BASEPATH}check64bit.h \ + $${BASEPATH}checkassert.h \ $${BASEPATH}checkassignif.h \ $${BASEPATH}checkautovariables.h \ $${BASEPATH}checkbool.h \ @@ -42,6 +43,7 @@ HEADERS += $${BASEPATH}check.h \ SOURCES += $${BASEPATH}check64bit.cpp \ + $${BASEPATH}checkassert.cpp \ $${BASEPATH}checkassignif.cpp \ $${BASEPATH}checkautovariables.cpp \ $${BASEPATH}checkbool.cpp \ diff --git a/test/testassert.cpp b/test/testassert.cpp new file mode 100644 index 000000000..32c649df4 --- /dev/null +++ b/test/testassert.cpp @@ -0,0 +1,141 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "tokenize.h" +#include "checkassert.h" +#include "testsuite.h" +#include + +extern std::ostringstream errout; + +class TestAssert : public TestFixture { +public: + TestAssert() : TestFixture("TestAsserts") {} + +private: + void check( + const char code[], + const char *filename = NULL) { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("style"); + settings.addEnabled("warning"); + settings.addEnabled("portability"); + settings.addEnabled("performance"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, filename ? filename : "test.cpp"); + + // Check.. + CheckAssert checkAssert(&tokenizer, &settings, this); + checkAssert.runSimplifiedChecks(&tokenizer, &settings, this); + } + + void run() { + TEST_CASE(assignmentInAssert); + TEST_CASE(functionCallInAssert); + TEST_CASE(safeFunctionCallInAssert); + } + + + void safeFunctionCallInAssert() { + check( + "int a;\n" + "bool b = false;\n" + "int foo() {\n" + " if (b) { a = 1+2 };\n" + " return a;\n" + "}\n" + "assert(foo() == 3); \n" + ); + ASSERT_EQUALS("", errout.str()); + } + + void functionCallInAssert() { + check( + "int a;\n" + "int foo() {\n" + " a = 1+2;\n" + " return a;\n" + "}\n" + "assert(foo() == 3); \n" + ); + ASSERT_EQUALS("[test.cpp:6]: (warning) Assert statement calls a function which may have desired side effects: 'foo'.\n", errout.str()); + } + + void assignmentInAssert() { + check("void f() {\n" + " int a; a = 0;\n" + " assert(a = 2);\n" + " return a;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); + + check("void f(int a) {\n" + " assert(a == 2);\n" + " return a;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(int a, int b) {\n" + " assert(a == 2 && b = 1);\n" + " return a;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Assert statement modifies 'b'.\n", errout.str()); + + check("void f() {\n" + " int a; a = 0;\n" + " assert(a += 2);\n" + " return a;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); + + check("void f() {\n" + " int a; a = 0;\n" + " assert(a *= 2);\n" + " return a;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); + + check("void f() {\n" + " int a; a = 0;\n" + " assert(a -= 2);\n" + " return a;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); + + check("void f() {\n" + " int a = 0;\n" + " assert(a--);\n" + " return a;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); + } +}; + +REGISTER_TEST(TestAssert) \ No newline at end of file diff --git a/test/testfiles.pri b/test/testfiles.pri index 92a601643..b4b56ec99 100644 --- a/test/testfiles.pri +++ b/test/testfiles.pri @@ -4,6 +4,7 @@ INCLUDEPATH += ../externals/tinyxml SOURCES += $${BASEPATH}/test64bit.cpp \ + $${BASEPATH}/testassert.cpp \ $${BASEPATH}/testassignif.cpp \ $${BASEPATH}/testautovariables.cpp \ $${BASEPATH}/testbool.cpp \ diff --git a/test/testother.cpp b/test/testother.cpp index d9392642e..b9f52a91d 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -106,9 +106,6 @@ private: TEST_CASE(trac2071); TEST_CASE(trac2084); TEST_CASE(trac3693); - - TEST_CASE(assignmentInAssert); - TEST_CASE(modulo); TEST_CASE(incorrectLogicOperator1); @@ -3235,62 +3232,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void assignmentInAssert() { - check("void f() {\n" - " int a; a = 0;\n" - " assert(a = 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f(int a) {\n" - " assert(a == 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int a, int b) {\n" - " assert(a == 2 && b = 1);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Assert statement modifies 'b'.\n", errout.str()); - - check("void f() {\n" - " int a; a = 0;\n" - " assert(a += 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f() {\n" - " int a; a = 0;\n" - " assert(a *= 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f() {\n" - " int a; a = 0;\n" - " assert(a -= 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f() {\n" - " int a = 0;\n" - " assert(a--);\n" // don't simplify and verify - " return a;\n" - "}\n", 0, false, false, false, false - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - } - void modulo() { check("bool f(bool& b1, bool& b2, bool& b3) {\n" " b1 = a % 5 == 4;\n" @@ -6224,4 +6165,4 @@ private: } }; -REGISTER_TEST(TestOther) +REGISTER_TEST(TestOther) \ No newline at end of file diff --git a/test/testrunner.vcxproj b/test/testrunner.vcxproj index fe2a85217..7d0129584 100644 --- a/test/testrunner.vcxproj +++ b/test/testrunner.vcxproj @@ -31,6 +31,7 @@ + diff --git a/test/testrunner.vcxproj.filters b/test/testrunner.vcxproj.filters index 0173d744b..7b0dd0123 100644 --- a/test/testrunner.vcxproj.filters +++ b/test/testrunner.vcxproj.filters @@ -172,6 +172,9 @@ Source Files + + Source Files +