From fafd0742d4e1e97214bf7096fbf4365359b93679 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 31 Oct 2018 03:47:48 -0500 Subject: [PATCH] Fix FPs with return conditions (#1455) * Fix 8815: FP with identical inner conditions * Fix issue 8801: FP when not returning a bool * Fix FP * Add missing semicolon * Move returnVar --- lib/checkcondition.cpp | 22 ++++++---------------- test/testcondition.cpp | 38 ++++++++++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 8bd480f6d..db29bc908 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -574,16 +574,7 @@ void CheckCondition::multiCondition2() // Condition.. const Token *cond2 = tok->str() == "if" ? condStartToken->astOperand2() : condStartToken->astOperand1(); - // Check if returning boolean values - if (tok->str() == "return") { - const Variable * condVar = nullptr; - if (Token::Match(cond2, "%var% ;")) - condVar = cond2->variable(); - else if (Token::Match(cond2, ". %var% ;")) - condVar = cond2->next()->variable(); - if (condVar && (condVar->isClass() || condVar->isPointer())) - break; - } + const bool isReturnVar = (tok->str() == "return" && !Token::Match(cond2, "%cop%")); ErrorPath errorPath; @@ -599,10 +590,10 @@ void CheckCondition::multiCondition2() tokens1.push(firstCondition->astOperand1()); tokens1.push(firstCondition->astOperand2()); } else if (!firstCondition->hasKnownValue()) { - if (isOppositeCond(false, mTokenizer->isCPP(), firstCondition, cond2, mSettings->library, true, true, &errorPath)) { + if (!isReturnVar && isOppositeCond(false, mTokenizer->isCPP(), firstCondition, cond2, mSettings->library, true, true, &errorPath)) { if (!isAliased(vars)) oppositeInnerConditionError(firstCondition, cond2, errorPath); - } else if (isSameExpression(mTokenizer->isCPP(), true, firstCondition, cond2, mSettings->library, true, true, &errorPath)) { + } else if (!isReturnVar && isSameExpression(mTokenizer->isCPP(), true, firstCondition, cond2, mSettings->library, true, true, &errorPath)) { identicalInnerConditionError(firstCondition, cond2, errorPath); } } @@ -1288,6 +1279,9 @@ void CheckCondition::alwaysTrueFalse() if (!(constIfWhileExpression || constValExpr || compExpr || returnStatement)) continue; + if (returnStatement && scope->function && !Token::simpleMatch(scope->function->retDef, "bool")) + continue; + if (returnStatement && isConstVarExpression(tok)) continue; @@ -1298,10 +1292,6 @@ void CheckCondition::alwaysTrueFalse() !isVariableChanged(tok->variable(), mSettings, mTokenizer->isCPP()))) continue; - // FIXME checking of return statements does not work well. See #8801 - if (returnStatement) - continue; - // Don't warn in assertions. Condition is often 'always true' by intention. // If platform,defines,etc cause 'always false' then that is not dangerous neither. bool assertFound = false; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index abd683787..7df1a8dbf 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -1656,6 +1656,12 @@ private: " }\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("int * f(int * x, int * y) {\n" + " if(!x) return x;\n" + " return y;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void oppositeInnerConditionClass() { @@ -2069,18 +2075,30 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical inner 'if' condition is always true.\n", errout.str()); - check("bool f(bool a) {\n" - " if(a) { return a; }\n" + check("bool f(int a, int b) {\n" + " if(a == b) { return a == b; }\n" " return false;\n" "}\n"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (warning) Identical inner 'return' condition is always true.\n", errout.str()); + check("bool f(bool a) {\n" + " if(a) { return a; }\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("int* f(int* a, int * b) {\n" " if(a) { return a; }\n" " return b;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + check("int* f(std::shared_ptr a, std::shared_ptr b) {\n" + " if(a.get()) { return a.get(); }\n" + " return b.get();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("struct A { int * x; };\n" "int* f(A a, int * b) {\n" " if(a.x) { return a.x; }\n" @@ -2493,7 +2511,7 @@ private: " if(x == 0) { x++; return x == 0; } \n" " return false;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0' is always false\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0' is always false\n", errout.str()); check("void f() {\n" // #6898 (Token::expressionString) " int x = 0;\n" @@ -2611,19 +2629,31 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); + check("int f(void){return 1/abs(10);}"); + ASSERT_EQUALS("", errout.str()); + check("bool f() { \n" " int x = 0;\n" " return x; \n" "}\n"); ASSERT_EQUALS("", errout.str()); - check("int f() {\n" + check("bool f() {\n" " const int a = 50;\n" " const int b = 52;\n" " return a+b;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + check("int f() {\n" + " int a = 50;\n" + " int b = 52;\n" + " a++;\n" + " b++;\n" + " return a+b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("bool& g();\n" "bool f() {\n" " bool & b = g();\n"