diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 3af2200d1..f469c13cd 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2012,10 +2012,6 @@ void CheckOther::checkSignOfUnsignedVariable() if (!mSettings->isEnabled(Settings::STYLE)) return; - const bool inconclusive = mTokenizer->codeWithTemplates(); - if (inconclusive && !mSettings->inconclusive) - return; - const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * scope : symbolDatabase->functionScopes) { @@ -2024,80 +2020,64 @@ void CheckOther::checkSignOfUnsignedVariable() if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) continue; - if (Token::Match(tok, "<|<= 0") && tok->next() == tok->astOperand2()) { + const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0); + const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0); + + if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) { const ValueType* vt = tok->astOperand1()->valueType(); if (vt && vt->pointer) - pointerLessThanZeroError(tok, inconclusive); + pointerLessThanZeroError(tok, v2); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedLessThanZeroError(tok, tok->astOperand1()->expressionString(), inconclusive); - } else if (Token::Match(tok->previous(), "0 >|>=") && tok->previous() == tok->astOperand1()) { + unsignedLessThanZeroError(tok, v2, tok->astOperand1()->expressionString()); + } else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) { const ValueType* vt = tok->astOperand2()->valueType(); if (vt && vt->pointer) - pointerLessThanZeroError(tok, inconclusive); + pointerLessThanZeroError(tok, v1); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedLessThanZeroError(tok, tok->astOperand2()->expressionString(), inconclusive); - } else if (Token::simpleMatch(tok, ">= 0") && tok->next() == tok->astOperand2()) { + unsignedLessThanZeroError(tok, v1, tok->astOperand2()->expressionString()); + } else if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) { const ValueType* vt = tok->astOperand1()->valueType(); if (vt && vt->pointer) - pointerPositiveError(tok, inconclusive); + pointerPositiveError(tok, v2); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedPositiveError(tok, tok->astOperand1()->str(), inconclusive); - } else if (Token::simpleMatch(tok->previous(), "0 <=") && tok->previous() == tok->astOperand1()) { + unsignedPositiveError(tok, v2, tok->astOperand1()->expressionString()); + } else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) { const ValueType* vt = tok->astOperand2()->valueType(); if (vt && vt->pointer) - pointerPositiveError(tok, inconclusive); + pointerPositiveError(tok, v1); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedPositiveError(tok, tok->astOperand2()->str(), inconclusive); + unsignedPositiveError(tok, v1, tok->astOperand2()->expressionString()); } } } } -void CheckOther::unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive) +void CheckOther::unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value * v, const std::string &varname) { - if (inconclusive) { - reportError(tok, Severity::style, "unsignedLessThanZero", - "$symbol:" + varname + "\n" - "Checking if unsigned expression '$symbol' is less than zero. This might be a false warning.\n" - "Checking if unsigned expression '$symbol' is less than zero. An unsigned " - "variable will never be negative so it is either pointless or an error to check if it is. " - "It's not known if the used constant is a template parameter or not and therefore " - "this message might be a false warning.", CWE570, true); - } else { - reportError(tok, Severity::style, "unsignedLessThanZero", - "$symbol:" + varname + "\n" - "Checking if unsigned expression '$symbol' is less than zero.\n" - "The unsigned expression '$symbol' will never be negative so it " - "is either pointless or an error to check if it is.", CWE570, false); - } + reportError(getErrorPath(tok, v, "Unsigned less than zero"), Severity::style, "unsignedLessThanZero", + "$symbol:" + varname + "\n" + "Checking if unsigned expression '$symbol' is less than zero.\n" + "The unsigned expression '$symbol' will never be negative so it " + "is either pointless or an error to check if it is.", CWE570, false); } -void CheckOther::pointerLessThanZeroError(const Token *tok, bool inconclusive) +void CheckOther::pointerLessThanZeroError(const Token *tok, const ValueFlow::Value *v) { - reportError(tok, Severity::style, "pointerLessThanZero", - "A pointer can not be negative so it is either pointless or an error to check if it is.", CWE570, inconclusive); + reportError(getErrorPath(tok, v, "Pointer less than zero"), Severity::style, "pointerLessThanZero", + "A pointer can not be negative so it is either pointless or an error to check if it is.", CWE570, false); } -void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive) +void CheckOther::unsignedPositiveError(const Token *tok, const ValueFlow::Value * v, const std::string &varname) { - if (inconclusive) { - reportError(tok, Severity::style, "unsignedPositive", - "$symbol:" + varname + "\n" - "Unsigned variable '$symbol' can't be negative so it is unnecessary to test it.\n" - "The unsigned variable '$symbol' can't be negative so it is unnecessary to test it. " - "It's not known if the used constant is a " - "template parameter or not and therefore this message might be a false warning", CWE570, true); - } else { - reportError(tok, Severity::style, "unsignedPositive", - "$symbol:" + varname + "\n" - "Unsigned variable '$symbol' can't be negative so it is unnecessary to test it.", CWE570, false); - } + reportError(getErrorPath(tok, v, "Unsigned positive"), Severity::style, "unsignedPositive", + "$symbol:" + varname + "\n" + "Unsigned expression '$symbol' can't be negative so it is unnecessary to test it.", CWE570, false); } -void CheckOther::pointerPositiveError(const Token *tok, bool inconclusive) +void CheckOther::pointerPositiveError(const Token *tok, const ValueFlow::Value * v) { - 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.", CWE570, inconclusive); + reportError(getErrorPath(tok, v, "Pointer positive"), Severity::style, "pointerPositive", + "A pointer can not be negative so it is either pointless or an error to check if it is not.", CWE570, false); } /* check if a constructor in given class scope takes a reference */ diff --git a/lib/checkother.h b/lib/checkother.h index e76bfd5a0..4d3802cac 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -252,10 +252,10 @@ private: void duplicateExpressionTernaryError(const Token *tok, ErrorPath errors); void duplicateBreakError(const Token *tok, bool inconclusive); void unreachableCodeError(const Token* tok, bool inconclusive); - 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 unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value *v, const std::string &varname); + void pointerLessThanZeroError(const Token *tok, const ValueFlow::Value *v); + void unsignedPositiveError(const Token *tok, const ValueFlow::Value *v, const std::string &varname); + void pointerPositiveError(const Token *tok, const ValueFlow::Value *v); void SuspiciousSemicolonError(const Token *tok); void negativeBitwiseShiftError(const Token *tok, int op); void redundantCopyError(const Token *tok, const std::string &varname); @@ -318,10 +318,10 @@ private: c.duplicateExpressionTernaryError(nullptr, errorPath); c.duplicateBreakError(nullptr, false); c.unreachableCodeError(nullptr, false); - c.unsignedLessThanZeroError(nullptr, "varname", false); - c.unsignedPositiveError(nullptr, "varname", false); - c.pointerLessThanZeroError(nullptr, false); - c.pointerPositiveError(nullptr, false); + c.unsignedLessThanZeroError(nullptr, nullptr, "varname"); + c.unsignedPositiveError(nullptr, nullptr, "varname"); + c.pointerLessThanZeroError(nullptr, nullptr); + c.pointerPositiveError(nullptr, nullptr); c.SuspiciousSemicolonError(nullptr); c.incompleteArrayFillError(nullptr, "buffer", "memset", false); c.varFuncNullUBError(nullptr); diff --git a/test/testother.cpp b/test/testother.cpp index 751974c0c..7af932535 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -224,7 +224,7 @@ private: TEST_CASE(constArgument); } - void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { + void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = 0) { // Clear the error buffer.. errout.str(""); @@ -239,6 +239,7 @@ private: settings->standards.cpp = Standards::CPPLatest; settings->inconclusive = inconclusive; settings->experimental = experimental; + settings->verbose = verbose; // Tokenize.. Tokenizer tokenizer(settings, this); @@ -256,7 +257,7 @@ private: } void check(const char code[], Settings *s) { - check(code,"test.cpp",false,true,true,s); + check(code,"test.cpp",false,true,true,false,s); } void checkP(const char code[], const char *filename = "test.cpp") { @@ -305,6 +306,7 @@ private: false, // experimental false, // inconclusive true, // runSimpleChecks + false, // verbose &settings); } @@ -312,7 +314,7 @@ private: static Settings settings; settings.platformType = Settings::Win32A; - check(code, nullptr, false, false, true, &settings); + check(code, nullptr, false, false, true, false, &settings); } void emptyBrackets() { @@ -2512,7 +2514,7 @@ private: check("void foo() {\n" " exit(0);\n" " break;\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary.\n", errout.str()); check("class NeonSession {\n" @@ -2521,16 +2523,16 @@ private: "void NeonSession::exit()\n" "{\n" " SAL_INFO(\"ucb.ucp.webdav\", \"neon commands cannot be aborted\");\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); check("void NeonSession::exit()\n" "{\n" " SAL_INFO(\"ucb.ucp.webdav\", \"neon commands cannot be aborted\");\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); - check("void foo() { xResAccess->exit(); }", nullptr, false, false, false, &settings); + check("void foo() { xResAccess->exit(); }", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); check("void foo(int a)\n" @@ -2773,20 +2775,20 @@ private: check("void foo() {\n" " (beat < 100) ? (void)0 : exit(0);\n" " bar();\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); check("void foo() {\n" " (beat < 100) ? exit(0) : (void)0;\n" " bar();\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); // #8261 check("void foo() {\n" " (beat < 100) ? (void)0 : throw(0);\n" " bar();\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); } @@ -3696,6 +3698,7 @@ private: false, // experimental false, // inconclusive false, // runSimpleChecks + false, // verbose nullptr // settings ); ASSERT_EQUALS("", errout.str()); @@ -3885,7 +3888,7 @@ private: check("void foo() {\n" " if ((mystrcmp(a, b) == 0) || (mystrcmp(a, b) == 0)) {}\n" - "}", "test.cpp", false, false, true, &settings); + "}", "test.cpp", false, false, true, false, &settings); ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); check("void GetValue() { return rand(); }\n" @@ -4782,18 +4785,31 @@ private: " for(unsigned char i = 10; i >= 0; i--)" " printf(\"%u\", i);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "void foo(bool b) {\n" " for(unsigned int i = 10; b || i >= 0; i--)" " printf(\"%u\", i);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); + + { + const char code[] = + "bool foo(unsigned int x) {\n" + " if (x < 0)" + " return true;\n" + " return false;\n" + "}"; + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + } check( "bool foo(unsigned int x) {\n" - " if (x < 0)" + " if (x < 0u)" " return true;\n" " return false;\n" "}"); @@ -4807,6 +4823,30 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + { + const char code[] = + "bool foo(unsigned x) {\n" + " int y = 0;\n" + " if (x < y)" + " return true;\n" + " return false;\n" + "}"; + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + } + check( + "bool foo(unsigned x) {\n" + " int y = 0;\n" + " if (b)\n" + " y = 1;\n" + " if (x < y)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check( "bool foo(unsigned int x) {\n" " if (0 > x)" @@ -4815,6 +4855,14 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check( + "bool foo(unsigned int x) {\n" + " if (0UL > x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check( "bool foo(int x) {\n" " if (0 > x)" @@ -4829,7 +4877,23 @@ private: " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(unsigned int x, unsigned y) {\n" + " if (x - y >= 0)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x-y' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(unsigned int x) {\n" + " if (x >= 0ull)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x) {\n" @@ -4839,6 +4903,29 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + check( + "bool foo(unsigned int x) {\n" + " if (0 <= x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(unsigned int x) {\n" + " if (0ll <= x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(int x) {\n" + " if (0 <= x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); check( "bool foo(unsigned int x, bool y) {\n" @@ -4878,7 +4965,7 @@ private: " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x, bool y) {\n" @@ -4927,7 +5014,7 @@ private: " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x, bool y) {\n" @@ -4976,7 +5063,7 @@ private: " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x, bool y) {\n" @@ -4996,8 +5083,25 @@ private: check(code, nullptr, false, false); ASSERT_EQUALS("", errout.str()); check(code, nullptr, false, true); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Checking if unsigned expression 'x' is less than zero. This might be a false warning.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); } + + check( + "template void foo(unsigned int x) {\n" + "if (x <= 0);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + + // #8836 + check( + "uint32_t value = 0xFUL;\n" + "void f() {\n" + " if (value < 0u)\n" + " {\n" + " value = 0u;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'value' is less than zero.\n", errout.str()); } void checkSignOfPointer() { @@ -5008,6 +5112,18 @@ private: "}"); 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()); + { + const char code[] = + "bool foo(int* x) {\n" + " int y = 0;\n" + " if (x >= y)" + " bar();\n" + "}"; + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:3]: (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(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (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( "bool foo(int* x) {\n" " if (*x >= 0)" @@ -5022,6 +5138,20 @@ private: "}"); 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()); + { + const char code[] = + "bool foo(int* x) {\n" + " unsigned y = 0u;\n" + " if (x < y)" + " bar();\n" + "}"; + + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + } + check( "bool foo(int* x) {\n" " if (*x < 0)"