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

@@ -574,16 +574,7 @@ void CheckCondition::multiCondition2()
 
                     // Condition..
                     const Token *cond2 = tok->str() == "if" ? condStartToken->astOperand2() : condStartToken->astOperand1();
-                    // Check if returning boolean values
+                    const bool isReturnVar = (tok->str() == "return" && !Token::Match(cond2, "%cop%"));
-                    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;
-                    }
 
                     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;

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"
+        check("bool f(int a, int b) {\n"
-              "    if(a) { return a; }\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<int> a, std::shared_ptr<int> 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"