From 85b2bd21dcdb201cc11cd1e4c0ac14283c7dd730 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sat, 6 Aug 2011 19:23:09 -0400 Subject: [PATCH] fix #2968 (new check: testing if unsigned variable is less than 0) --- lib/checkother.cpp | 60 +++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 11 +++++++- test/testother.cpp | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index e545b32e1..34f9ce539 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3866,3 +3866,63 @@ void CheckOther::assignBoolToPointerError(const Token *tok) reportError(tok, Severity::error, "assignBoolToPointer", "Assigning bool value to pointer (converting bool value to address)"); } + +//--------------------------------------------------------------------------- +// Check testing sign of unsigned variables. +//--------------------------------------------------------------------------- + +void CheckOther::checkSignOfUnsignedVariable() +{ + if (!_settings->_checkCodingStyle) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + + std::list::const_iterator scope; + + for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) + { + // only check functions + if (scope->type != Scope::eFunction) + continue; + + // check all the code in the function + for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) + { + if (Token::Match(tok, "( %var% <|<= 0 )") && tok->next()->varId()) + { + const Variable * var = symbolDatabase->getVariableFromVarId(tok->next()->varId()); + if (var && var->typeEndToken()->isUnsigned()) + unsignedLessThanZero(tok->next(), tok->next()->str()); + } + else if (Token::Match(tok, "( 0 > %var% )") && tok->tokAt(3)->varId()) + { + const Variable * var = symbolDatabase->getVariableFromVarId(tok->tokAt(3)->varId()); + if (var && var->typeEndToken()->isUnsigned()) + unsignedLessThanZero(tok->tokAt(3), tok->strAt(3)); + } + else if (Token::Match(tok, "( 0 <= %var% )") && tok->tokAt(3)->varId()) + { + const Variable * var = symbolDatabase->getVariableFromVarId(tok->tokAt(3)->varId()); + if (var && var->typeEndToken()->isUnsigned()) + unsignedPositive(tok->tokAt(3), tok->strAt(3)); + } + } + } +} + +void CheckOther::unsignedLessThanZero(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::style, "unsignedLessThanZero", + "Checking if unsigned variable '" + varname + "' is less than zero.\n" + "An unsigned variable will never be negative so it is either pointless or " + "an error to check if it is."); +} + +void CheckOther::unsignedPositive(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::style, "unsignedPositive", + "Checking if unsigned variable '" + varname + "' is positive is always true.\n" + "An unsigned variable will never alwayw be positive so it is either pointless or " + "an error to check if it is."); +} diff --git a/lib/checkother.h b/lib/checkother.h index 7cd95f77f..f3ddb09c0 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -103,6 +103,7 @@ public: checkOther.checkAlwaysTrueOrFalseStringCompare(); checkOther.checkAssignBoolToPointer(); + checkOther.checkSignOfUnsignedVariable(); } /** @brief Clarify calculation for ".. a * b ? .." */ @@ -233,10 +234,12 @@ public: /** @brief check if token is a record type without side effects */ bool isRecordTypeWithoutSideEffects(const Token *tok); - /** @brief assigning bool to pointer */ void checkAssignBoolToPointer(); + /** @brief %Check for testing sign of unsigned variable */ + void checkSignOfUnsignedVariable(); + // Error messages.. void cstyleCastError(const Token *tok); void dangerousUsageStrtolError(const Token *tok); @@ -272,6 +275,8 @@ public: void alwaysTrueFalseStringCompare(const Token *tok, const std::string& str1, const std::string& str2); void duplicateBreakError(const Token *tok); void assignBoolToPointerError(const Token *tok); + void unsignedLessThanZero(const Token *tok, const std::string &varname); + void unsignedPositive(const Token *tok, const std::string &varname); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { @@ -323,6 +328,8 @@ public: c.duplicateExpressionError(0, 0, "&&"); c.alwaysTrueFalseStringCompare(0, "str1", "str2"); c.duplicateBreakError(0); + c.unsignedLessThanZero(0, "varname"); + c.unsignedPositive(0, "varname"); } std::string myName() const @@ -370,6 +377,8 @@ public: "* suspicious condition (assignment+comparison)\n" "* suspicious condition (runtime comparison of string literals)\n" "* duplicate break statement\n" + "* testing if unsigned variable is negative\n" + "* testing is unsigned variable is positive\n" // optimisations "* optimisation: detect post increment/decrement\n"; diff --git a/test/testother.cpp b/test/testother.cpp index b0a7cbf61..577f5718c 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -127,6 +127,7 @@ private: TEST_CASE(duplicateExpression2); // ticket #2730 TEST_CASE(alwaysTrueFalseStringCompare); + TEST_CASE(checkSignOfUnsignedVariable); } void check(const char code[], const char *filename = NULL) @@ -2888,6 +2889,75 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void check_signOfUnsignedVariable(const char code[]) + { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings._checkCodingStyle = true; + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Check for redundant code.. + CheckOther checkOther(&tokenizer, &settings, this); + checkOther.checkSignOfUnsignedVariable(); + } + + void checkSignOfUnsignedVariable() + { + check_signOfUnsignedVariable( + "bool foo(unsigned int x) {\n" + " if (x < 0)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned variable 'x' is less than zero.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int x) {\n" + " if (x < 0)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(unsigned int x) {\n" + " if (0 > x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned variable 'x' is less than zero.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int x) {\n" + " if (0 > x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(unsigned int x) {\n" + " if (x >= 0)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned variable 'x' is positive is always true.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int x) {\n" + " if (x >= 0)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)