Refactorization: Support function default values in ValueFlow, removed now obsolete CheckNullPointer::nullPointerDefaultArgument().

-> Use valueFlowForward() to parse values passed to functions
-> valueFlowForward(): Set value in first occurrence of a variable in a condition

lib/checknullpointer.cpp

@@ -327,7 +327,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
             parseFunctionCall(*ftok->previous(), varlist, &_settings->library, 0);
             if (std::find(varlist.begin(), varlist.end(), tok) != varlist.end()) {
                 if (value->condition == nullptr)
-                    nullPointerError(tok, tok->str());
+                    nullPointerError(tok, tok->str(), false, value->defaultArg);
                 else if (_settings->isEnabled("warning"))
                     nullPointerError(tok, tok->str(), value->condition, value->inconclusive);
             }
@@ -339,7 +339,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
         if (!isPointerDeRef(tok,unknown)) {
             if (_settings->inconclusive && unknown) {
                 if (value->condition == nullptr)
-                    nullPointerError(tok, tok->str(), true);
+                    nullPointerError(tok, tok->str(), true, value->defaultArg);
                 else
                     nullPointerError(tok, tok->str(), value->condition, true);
             }
@@ -347,7 +347,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
         }
 
         if (value->condition == nullptr)
-            nullPointerError(tok, tok->str(), value->inconclusive);
+            nullPointerError(tok, tok->str(), value->inconclusive, value->defaultArg);
         else if (_settings->isEnabled("warning"))
             nullPointerError(tok, tok->str(), value->condition, value->inconclusive);
     }
@@ -357,7 +357,6 @@ void CheckNullPointer::nullPointer()
 {
     nullPointerLinkedList();
     nullPointerByDeRefAndChec();
-    nullPointerDefaultArgument();
 }
 
 /** Dereferencing null constant (simplified token list) */
@@ -444,158 +443,17 @@ void CheckNullPointer::nullConstantDereference()
     }
 }
 
-/**
-* @brief If tok is a function call that passes in a pointer such that
-*         the pointer may be modified, this function will remove that
-*         pointer from pointerArgs.
-*/
-void CheckNullPointer::removeAssignedVarFromSet(const Token* tok, std::set<unsigned int>& pointerArgs)
-{
-    // If a pointer's address is passed into a function, stop considering it
-    if (Token::Match(tok->previous(), "[;{}] %name% (")) {
-        const Token* endParen = tok->next()->link();
-        for (const Token* tok2 = tok->next(); tok2 != endParen; tok2 = tok2->next()) {
-            if (tok2->isName() && tok2->varId() > 0
-                // Common functions that are known NOT to modify their pointer argument
-                && !Token::Match(tok, "printf|sprintf|fprintf|vprintf")) {
-                pointerArgs.erase(tok2->varId());
-            }
-        }
-    }
-}
-
-/**
-* @brief Does one part of the check for nullPointer().
-* -# default argument that sets a pointer to 0
-* -# dereference pointer
-*/
-void CheckNullPointer::nullPointerDefaultArgument()
-{
-    if (!_settings->isEnabled("warning"))
-        return;
-
-    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
-    const std::size_t functions = symbolDatabase->functionScopes.size();
-    for (std::size_t i = 0; i < functions; ++i) {
-        const Scope * scope = symbolDatabase->functionScopes[i];
-        if (scope->function == 0 || !scope->function->hasBody()) // We only look for functions with a body
-            continue;
-
-        // Scan the argument list for default arguments that are pointers and
-        // which default to a NULL pointer if no argument is specified.
-        std::set<unsigned int> pointerArgs;
-        for (const Token *tok = scope->function->arg; tok != scope->function->arg->link(); tok = tok->next()) {
-
-            if (Token::Match(tok, "%var% = 0 ,|)")) {
-                const Variable *var = tok->variable();
-                if (var && var->isPointer())
-                    pointerArgs.insert(tok->varId());
-            }
-        }
-
-        // Report an error if any of the default-NULL arguments are dereferenced
-        if (!pointerArgs.empty()) {
-            for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
-
-                // If we encounter a possible NULL-pointer check, skip over its body
-                if (tok->str() == "?") { // TODO: Skip this if the condition is unrelated to the variables
-                    // Find end of statement
-                    tok = tok->astOperand2();
-                    while (tok && !Token::Match(tok, ")|;")) {
-                        if (tok->link() && Token::Match(tok, "(|[|<|{"))
-                            tok = tok->link();
-                        tok = tok->next();
-                    }
-                    if (!tok)
-                        break;
-                } else if (Token::simpleMatch(tok, "if ("))  {
-                    bool dependsOnPointer = false;
-                    const Token *endOfCondition = tok->next()->link();
-                    if (!endOfCondition)
-                        continue;
-
-                    const Token *startOfIfBlock =
-                        Token::simpleMatch(endOfCondition, ") {") ? endOfCondition->next() : nullptr;
-                    if (!startOfIfBlock)
-                        continue;
-
-                    // If this if() statement may return, it may be a null
-                    // pointer check for the pointers referenced in its condition
-                    const Token *endOfIf = startOfIfBlock->link();
-                    bool isExitOrReturn =
-                        Token::findmatch(startOfIfBlock, "exit|return|throw", endOfIf) != nullptr;
-
-                    if (Token::Match(tok, "if ( %var% == 0 )")) {
-                        const unsigned int varid = tok->tokAt(2)->varId();
-                        if (pointerArgs.count(varid) > 0) {
-                            if (isExitOrReturn)
-                                pointerArgs.erase(varid);
-                            else
-                                dependsOnPointer = true;
-                        }
-                    } else {
-                        for (const Token *tok2 = tok->next(); tok2 != endOfCondition; tok2 = tok2->next()) {
-                            if (tok2->isName() && tok2->varId() > 0 &&
-                                pointerArgs.count(tok2->varId()) > 0) {
-
-                                // If the if() depends on a pointer and may return, stop
-                                // considering that pointer because it may be a NULL-pointer
-                                // check that returns if the pointer is NULL.
-                                if (isExitOrReturn)
-                                    pointerArgs.erase(tok2->varId());
-                                else
-                                    dependsOnPointer = true;
-                            }
-                        }
-                    }
-
-                    if (dependsOnPointer && endOfIf) {
-                        for (; tok != endOfIf; tok = tok->next()) {
-                            // If a pointer is assigned a new value, stop considering it.
-                            if (Token::Match(tok, "%var% ="))
-                                pointerArgs.erase(tok->varId());
-                            else
-                                removeAssignedVarFromSet(tok, pointerArgs);
-                        }
-                        continue;
-                    }
-                }
-
-                // If there is a noreturn function (e.g. exit()), stop considering the rest of
-                // this function.
-                bool unknown = false;
-                if (Token::Match(tok, "return|throw|exit") ||
-                    (_tokenizer->IsScopeNoReturn(tok, &unknown) && !unknown))
-                    break;
-
-                removeAssignedVarFromSet(tok, pointerArgs);
-
-                if (tok->varId() == 0 || pointerArgs.count(tok->varId()) == 0)
-                    continue;
-
-                // If a pointer is assigned a new value, stop considering it.
-                if (Token::Match(tok, "%var% ="))
-                    pointerArgs.erase(tok->varId());
-
-                // If a pointer dereference is preceded by an && or ||,
-                // they serve as a sequence point so the dereference
-                // may not be executed.
-                if (isPointerDeRef(tok, unknown) && !unknown &&
-                    tok->strAt(-1) != "&&" && tok->strAt(-1) != "||" &&
-                    tok->strAt(-2) != "&&" && tok->strAt(-2) != "||")
-                    nullPointerDefaultArgError(tok, tok->str());
-            }
-        }
-    }
-}
-
 void CheckNullPointer::nullPointerError(const Token *tok)
 {
     reportError(tok, Severity::error, "nullPointer", "Null pointer dereference");
 }
 
-void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, bool inconclusive)
+void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, bool inconclusive, bool defaultArg)
 {
+    if (defaultArg) {
+        if (_settings->isEnabled("warning"))
+            reportError(tok, Severity::warning, "nullPointer", "Possible null pointer dereference if the default parameter value is used: " + varname, inconclusive);
+    } else
         reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference: " + varname, inconclusive);
 }
 
@@ -607,8 +465,3 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var
     const std::string errmsg("Possible null pointer dereference: " + varname + " - otherwise it is redundant to check it against null.");
     reportError(callstack, Severity::warning, "nullPointer", errmsg, inconclusive);
 }
-
-void CheckNullPointer::nullPointerDefaultArgError(const Token *tok, const std::string &varname)
-{
-    reportError(tok, Severity::warning, "nullPointer", "Possible null pointer dereference if the default parameter value is used: " + varname);
-}

lib/checknullpointer.h

@@ -86,9 +86,8 @@ public:
     void nullConstantDereference();
 
     void nullPointerError(const Token *tok);  // variable name unknown / doesn't exist
-    void nullPointerError(const Token *tok, const std::string &varname, bool inconclusive=false);
+    void nullPointerError(const Token *tok, const std::string &varname, bool inconclusive = false, bool defaultArg = false);
     void nullPointerError(const Token *tok, const std::string &varname, const Token* nullcheck, bool inconclusive = false);
-    void nullPointerDefaultArgError(const Token *tok, const std::string &varname);
 private:
 
     /** Get error messages. Used by --errorlist */
@@ -119,26 +118,6 @@ private:
      * Dereferencing a pointer and then checking if it's NULL..
      */
     void nullPointerByDeRefAndChec();
-
-    /**
-     * @brief Does one part of the check for nullPointer().
-     * -# initialize pointer to 0
-     * -# conditionally assign pointer
-     * -# dereference pointer
-     */
-    void nullPointerConditionalAssignment();
-
-    /**
-     * @brief Does one part of the check for nullPointer().
-     * -# default argument that sets a pointer to 0
-     * -# dereference pointer
-     */
-    void nullPointerDefaultArgument();
-
-    /**
-     * @brief Removes any variable that may be assigned from pointerArgs.
-     */
-    static void removeAssignedVarFromSet(const Token* tok, std::set<unsigned int>& pointerArgs);
 };
 /// @}
 //---------------------------------------------------------------------------

lib/valueflow.cpp

@@ -757,6 +757,15 @@ static bool valueFlowForward(Token * const               startToken,
                 return false;
             }
 
+            // Set values in condition
+            for (Token* tok3 = tok2->tokAt(2); tok3 != tok2->next()->link(); tok3 = tok3->next()) {
+                if (tok3->varId() == varid) {
+                    for (std::list<ValueFlow::Value>::const_iterator it = values.begin(); it != values.end(); ++it)
+                        setTokenValue(tok3, *it);
+                    break;
+                }
+            }
+
             // Should scope be skipped because variable value is checked?
             std::list<ValueFlow::Value> truevalues;
             for (std::list<ValueFlow::Value>::iterator it = values.begin(); it != values.end(); ++it) {
@@ -1560,6 +1569,20 @@ static void valueFlowForLoop(TokenList *tokenlist, SymbolDatabase* symboldatabas
     }
 }
 
+static void valueFlowInjectParameter(TokenList* tokenlist, ErrorLogger* errorLogger, const Settings* settings, const Variable* arg, const Scope* functionScope, const std::list<ValueFlow::Value>& argvalues)
+{
+    // Is argument passed by value or const reference, and is it a known non-class type?
+    if (arg->isReference() && !arg->isConst() && !arg->isClass())
+        return;
+
+    // Set value in function scope..
+    const unsigned int varid2 = arg->declarationId();
+    if (!varid2)
+        return;
+
+    valueFlowForward(const_cast<Token*>(functionScope->classStart->next()), functionScope->classEnd, arg, varid2, argvalues, true, tokenlist, errorLogger, settings);
+}
+
 static void valueFlowSubFunction(TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings)
 {
     for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
@@ -1603,24 +1626,28 @@ static void valueFlowSubFunction(TokenList *tokenlist, ErrorLogger *errorLogger,
                     continue;
                 }
             }
-
+            valueFlowInjectParameter(tokenlist, errorLogger, settings, arg, functionScope, argvalues);
-            // Is argument passed by value?
-            if (!Token::Match(arg->typeStartToken(), "%type% %var% ,|)") &&
-                !Token::Match(arg->typeStartToken(), "const| struct| %type% * %var% ,|)"))
-                continue;
-
-            // Set value in function scope..
-            const unsigned int varid2 = arg->declarationId();
-            for (const Token *tok2 = functionScope->classStart->next(); tok2 != functionScope->classEnd; tok2 = tok2->next()) {
-                if (Token::Match(tok2, "%varid% !!=", varid2)) {
-                    for (std::list<ValueFlow::Value>::const_iterator val = argvalues.begin(); val != argvalues.end(); ++val)
-                        setTokenValue(const_cast<Token*>(tok2), *val);
-                } else if (Token::Match(tok2, "%oror%|&&|{|?")) {
-                    if (settings->debugwarnings)
-                        bailout(tokenlist, errorLogger, tok2, "parameter " + arg->name() + ", at '" + tok2->str() + "'");
-                    break;
         }
     }
+}
+
+static void valueFlowFunctionDefaultParameter(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
+{
+    if (!tokenlist->isCPP())
+        return;
+
+    const std::size_t functions = symboldatabase->functionScopes.size();
+    for (std::size_t i = 0; i < functions; ++i) {
+        const Scope* scope = symboldatabase->functionScopes[i];
+        const Function* function = scope->function;
+        if (!function)
+            continue;
+        for (std::size_t arg = function->minArgCount(); arg < function->argCount(); arg++) {
+            const Variable* var = function->getArgumentVar(arg);
+            if (var && var->hasDefault() && Token::Match(var->nameToken(), "%var% = %num%|%str% [,)]")) {
+                const_cast<Token*>(var->nameToken()->tokAt(2))->values.front().defaultArg = true;
+                valueFlowInjectParameter(tokenlist, errorLogger, settings, var, scope, var->nameToken()->tokAt(2)->values);
+            }
         }
     }
 }
@@ -1704,4 +1731,5 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
     valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);
     valueFlowAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
     valueFlowSubFunction(tokenlist, errorLogger, settings);
+    valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings);
 }

lib/valueflow.h

@@ -30,8 +30,8 @@ class Settings;
 namespace ValueFlow {
     class Value {
     public:
-        Value(long long val = 0) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false) {}
+        Value(long long val = 0) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {}
-        Value(const Token *c, long long val) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false) {}
+        Value(const Token *c, long long val) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {}
 
         /** int value */
         long long intvalue;
@@ -53,6 +53,9 @@ namespace ValueFlow {
 
         /** Is this value inconclusive? */
         bool inconclusive;
+
+        /** Is this value inconclusive? */
+        bool defaultArg;
     };
 
     void setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings);

test/testnullpointer.cpp

@@ -2306,8 +2306,8 @@ private:
         check("void f(int *p = 0) {\n"
               "    if (a != 0)\n"
               "      *p = 0;\n"
-              "}", false, "test.cpp", false);
+              "}", true, "test.cpp", false);
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
+        TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str());
 
         check("void f(int *p = 0) {\n"
               "    p = a;\n"
@@ -2376,8 +2376,8 @@ private:
         check("void f(int *p = 0) {\n"
               "    printf(\"%d\", p);\n"
               "    *p = 0;\n"
-              "}");
+              "}", true);
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
 
         // The init() function may or may not initialize p, but since the address
         // of p is passed in, it's a good bet that p may be modified and
@@ -2414,7 +2414,7 @@ private:
         check("void foo(int *p = 0) {\n"
               "    int var1 = x ? *p : 5;\n"
               "}");
-        TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str());
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
     }
 
     void nullpointer_internal_error() { // ticket #5080

test/testsimplifytypedef.cpp

@@ -1103,7 +1103,7 @@ private:
                       "[test.cpp:20] -> [test.cpp:1]: (style, inconclusive) The function parameter 'A' hides a typedef with the same name.\n"
                       "[test.cpp:21] -> [test.cpp:1]: (style, inconclusive) The variable 'A' hides a typedef with the same name.\n"
                       "[test.cpp:24] -> [test.cpp:1]: (style, inconclusive) The typedef 'A' hides a typedef with the same name.\n"
-                      "[test.cpp:24]: (debug) ValueFlow bailout: parameter a, at '{'\n", errout.str());
+                      "[test.cpp:21]: (debug) ValueFlow bailout: increment/decrement of a\n", errout.str());
     }
 
     void simplifyTypedef36() {

test/testvalueflow.cpp

@@ -1120,7 +1120,7 @@ private:
                "    if (x && \n"
                "        x->str() != y) {}\n"
                "}";
-        TODO_ASSERT_EQUALS(true, false, testValueOfX(code, 3U, 0));
+        ASSERT_EQUALS(true, testValueOfX(code, 3U, 0));
         ASSERT_EQUALS(false, testValueOfX(code, 4U, 0));
 
         // return