diff --git a/lib/checkother.cpp b/lib/checkother.cpp index e5f7bd946..7eaa0decf 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2921,7 +2921,7 @@ void CheckOther::comparisonOfBoolExpressionWithIntError(const Token *tok, bool n //--------------------------------------------------------------------------- -// Check testing sign of unsigned variables. +// Check testing sign of unsigned variables and pointers. //--------------------------------------------------------------------------- void CheckOther::checkSignOfUnsignedVariable() { @@ -2942,23 +2942,31 @@ void CheckOther::checkSignOfUnsignedVariable() continue; // check all the code in the function - for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) { + for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { if (Token::Match(tok, "%var% <|<= 0") && tok->varId() && !Token::Match(tok->previous(), "++|--|)|+|-|*|/|~|<<|>>") && !Token::Match(tok->tokAt(3), "+|-")) { const Variable * var = symbolDatabase->getVariableFromVarId(tok->varId()); if (var && var->typeEndToken()->isUnsigned()) - unsignedLessThanZeroError(tok, tok->str(), inconclusive); + unsignedLessThanZeroError(tok, var->name(), inconclusive); + else if (var && var->isPointer() && tok->strAt(-1) != "*") + pointerLessThanZeroError(tok, inconclusive); } else if (Token::Match(tok, "0 >|>= %var%") && tok->tokAt(2)->varId() && !Token::Match(tok->tokAt(3), "+|-|*|/") && !Token::Match(tok->previous(), "+|-|<<|>>|~")) { const Variable * var = symbolDatabase->getVariableFromVarId(tok->tokAt(2)->varId()); if (var && var->typeEndToken()->isUnsigned()) - unsignedLessThanZeroError(tok, tok->strAt(2), inconclusive); + unsignedLessThanZeroError(tok, var->name(), inconclusive); + else if (var && var->isPointer() && !Token::Match(tok->tokAt(3), "[.[]")) + pointerLessThanZeroError(tok, inconclusive); } else if (Token::Match(tok, "0 <= %var%") && tok->tokAt(2)->varId() && !Token::Match(tok->tokAt(3), "+|-|*|/") && !Token::Match(tok->previous(), "+|-|<<|>>|~")) { const Variable * var = symbolDatabase->getVariableFromVarId(tok->tokAt(2)->varId()); if (var && var->typeEndToken()->isUnsigned()) - unsignedPositiveError(tok, tok->strAt(2), inconclusive); + unsignedPositiveError(tok, var->name(), inconclusive); + else if (var && var->isPointer() && !Token::Match(tok->tokAt(3), "[.[]")) + pointerPositiveError(tok, inconclusive); } else if (Token::Match(tok, "%var% >= 0") && tok->varId() && !Token::Match(tok->previous(), "++|--|)|+|-|*|/|~|<<|>>") && !Token::Match(tok->tokAt(3), "+|-")) { const Variable * var = symbolDatabase->getVariableFromVarId(tok->varId()); if (var && var->typeEndToken()->isUnsigned()) - unsignedPositiveError(tok, tok->str(), inconclusive); + unsignedPositiveError(tok, var->name(), inconclusive); + else if (var && var->isPointer() && tok->strAt(-1) != "*") + pointerPositiveError(tok, inconclusive); } } } @@ -2981,6 +2989,12 @@ void CheckOther::unsignedLessThanZeroError(const Token *tok, const std::string & } } +void CheckOther::pointerLessThanZeroError(const Token *tok, bool inconclusive) +{ + reportError(tok, Severity::style, "pointerLessThanZero", + "A pointer can not be negative so it is either pointless or an error to check if it is.", inconclusive); +} + void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive) { if (inconclusive) { @@ -2995,6 +3009,12 @@ void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varn } } +void CheckOther::pointerPositiveError(const Token *tok, bool inconclusive) +{ + reportError(tok, Severity::style, "pointerPositive", + "A pointer can not be negative so it is either pointless or an error to check if it is not.", inconclusive); +} + /* This check rule works for checking the "const A a = getA()" usage when getA() returns "const A &" or "A &". In most scenarios, "const A & a = getA()" will be more efficient. diff --git a/lib/checkother.h b/lib/checkother.h index 99b69f7a2..fc0354ebd 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -236,6 +236,8 @@ public: /** @brief %Check for double free or double close operations */ void checkDoubleFree(); void doubleFreeError(const Token *tok, const std::string &varname); + + /** @brief %Check for code creating redundant copies */ void checkRedundantCopy(); private: @@ -285,7 +287,9 @@ private: void unreachableCodeError(const Token* tok, bool inconclusive); void assignBoolToPointerError(const Token *tok); void unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive); + void pointerLessThanZeroError(const Token *tok, bool inconclusive); void unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive); + void pointerPositiveError(const Token *tok, bool inconclusive); void bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op); void comparisonOfBoolExpressionWithIntError(const Token *tok, bool n0o1); void SuspiciousSemicolonError(const Token *tok); @@ -346,6 +350,8 @@ private: c.unreachableCodeError(0, false); c.unsignedLessThanZeroError(0, "varname", false); c.unsignedPositiveError(0, "varname", false); + c.pointerLessThanZeroError(0, false); + c.pointerPositiveError(0, false); c.bitwiseOnBooleanError(0, "varname", "&&"); c.comparisonOfBoolExpressionWithIntError(0, true); c.SuspiciousSemicolonError(0); diff --git a/test/testother.cpp b/test/testother.cpp index 292914b26..d59ed0f35 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -153,6 +153,7 @@ private: TEST_CASE(alwaysTrueFalseStringCompare); TEST_CASE(checkPointerSizeof); TEST_CASE(checkSignOfUnsignedVariable); + TEST_CASE(checkSignOfPointer); TEST_CASE(checkForSuspiciousSemicolon1); TEST_CASE(checkForSuspiciousSemicolon2); @@ -4830,6 +4831,78 @@ private: } } + void checkSignOfPointer() { + check_signOfUnsignedVariable( + "bool foo(int* x) {\n" + " if (x >= 0)" + " bar();\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int* x) {\n" + " if (*x >= 0)" + " bar();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int* x) {\n" + " if (x < 0)" + " bar();\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int* x) {\n" + " if (*x < 0)" + " bar();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(Bar* x) {\n" + " if (0 <= x)" + " bar();\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int* x) {\n" + " if (0 <= x[0])" + " bar();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(Bar* x) {\n" + " if (0 <= x.y)" + " bar();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(Bar* x) {\n" + " if (0 > x)" + " bar();\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(int* x) {\n" + " if (0 > x[0])" + " bar();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_signOfUnsignedVariable( + "bool foo(Bar* x) {\n" + " if (0 > x.y)" + " bar();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void checkForSuspiciousSemicolon1() { check( "void foo() {\n"