Add new "style" check to catch redundant pointer operations

Doing "&*some_ptr_var" is redundant and might be the remainder
of a refactoring. Warnings for expanded macros are excluded though:
They are often used with and without pointers and
do something like this: "func(&(*macroarg))".

The new check is fully AST based and was given
strong false positive testing on a large code base.

lib/checkother.cpp

@@ -2853,3 +2853,40 @@ void CheckOther::ignoredReturnValueError(const Token* tok, const std::string& fu
     reportError(tok, Severity::warning, "ignoredReturnValue",
                 "Return value of function " + function + "() is not used.", false);
 }
+
+void CheckOther::checkRedundantPointerOp()
+{
+    if (!_settings->isEnabled("style"))
+        return;
+
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
+        if (tok->str() == "&") {
+            // bail out for logical AND operator
+            if (tok->astOperand2())
+                continue;
+
+            // pointer dereference
+            const Token *astTok = tok->astOperand1();
+            if (!astTok || astTok->str() != "*")
+                continue;
+
+            // variable
+            const Token *varTok = astTok->astOperand1();
+            if (!varTok || varTok->isExpandedMacro() || varTok->varId() == 0)
+                continue;
+
+            const Variable *var = symbolDatabase->getVariableFromVarId(varTok->varId());
+            if (!var || !var->isPointer())
+                continue;
+
+            redundantPointerOpError(tok, var->name(), false);
+        }
+    }
+}
+
+void CheckOther::redundantPointerOpError(const Token* tok, const std::string &varname, bool inconclusive)
+{
+    reportError(tok, Severity::style, "redundantPointerOp",
+                "Redundant pointer operation on " + varname + " - it's already a pointer.", inconclusive);
+}

lib/checkother.h

@@ -74,6 +74,7 @@ public:
         checkOther.checkNanInArithmeticExpression();
         checkOther.checkCommaSeparatedReturn();
         checkOther.checkIgnoredReturnValue();
+        checkOther.checkRedundantPointerOp();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -233,6 +234,9 @@ public:
     /** @brief %Check for ignored return values. */
     void checkIgnoredReturnValue();
 
+    /** @brief %Check for redundant pointer operations */
+    void checkRedundantPointerOp();
+
 private:
     // Error messages..
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result);
@@ -288,6 +292,7 @@ private:
     void varFuncNullUBError(const Token *tok);
     void commaSeparatedReturnError(const Token *tok);
     void ignoredReturnValueError(const Token* tok, const std::string& function);
+    void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
@@ -345,6 +350,7 @@ private:
         c.nanInArithmeticExpressionError(0);
         c.commaSeparatedReturnError(0);
         c.ignoredReturnValueError(0, "malloc");
+        c.redundantPointerOpError(0, "varname", false);
     }
 
     static std::string myName() {
@@ -402,7 +408,8 @@ private:
                "- NaN (not a number) value used in arithmetic expression.\n"
                "- comma in return statement (the comma can easily be misread as a semicolon).\n"
                "- prefer erfc, expm1 or log1p to avoid loss of precision.\n"
-               "- identical code in both branches of if/else or ternary operator.\n";
+               "- identical code in both branches of if/else or ternary operator.\n"
+               "- redundant pointer operation on pointer like &*some_ptr.\n";
     }
 };
 /// @}

test/testother.cpp

@@ -176,9 +176,11 @@ private:
 
         TEST_CASE(testReturnIgnoredReturnValue);
         TEST_CASE(testReturnIgnoredReturnValuePosix);
+
+        TEST_CASE(redundantPointerOp);
     }
 
-    void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool posix = false, bool runSimpleChecks=true, Settings* settings = 0) {
+    void check(const char raw_code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool posix = false, bool runSimpleChecks=true, Settings* settings = 0) {
         // Clear the error buffer..
         errout.str("");
 
@@ -197,6 +199,14 @@ private:
         settings->experimental = experimental;
         settings->standards.posix = posix;
 
+        // Preprocess file..
+        Preprocessor preprocessor(settings);
+        std::list<std::string> configurations;
+        std::string filedata = "";
+        std::istringstream fin(raw_code);
+        preprocessor.preprocess(fin, filedata, configurations, "", settings->_includePaths);
+        const std::string code = preprocessor.getcode(filedata, "", "");
+
         // Tokenize..
         Tokenizer tokenizer(settings, this);
         std::istringstream istr(code);
@@ -6455,6 +6465,62 @@ private:
              );
         ASSERT_EQUALS("[test.cpp:3]: (warning) Return value of function mmap() is not used.\n", errout.str());
     }
+
+    void redundantPointerOp() {
+        check("int *f(int *x) {\n"
+              "    return &*x;\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on x - it's already a pointer.\n", errout.str());
+
+        check("int *f(int *y) {\n"
+              "    return &(*y);\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on y - it's already a pointer.\n", errout.str());
+
+        // no warning for bitwise AND
+        check("void f(int *b) {\n"
+              "    int x = 0x20 & *b;\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("", errout.str());
+
+        // No message for double pointers to structs
+        check("void f(struct foo **my_struct) {\n"
+              "    char **pass_to_func = &(*my_struct)->buf;\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("", errout.str());
+
+        // another double pointer to struct - with an array
+        check("void f(struct foo **my_struct) {\n"
+              "    char **pass_to_func = &(*my_struct)->buf[10];\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("", errout.str());
+
+        // double pointer to array
+        check("void f(char **ptr) {\n"
+              "    int *x = &(*ptr)[10];\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("", errout.str());
+
+        // function calls
+        check("void f(Mutex *mut) {\n"
+              "    pthread_mutex_lock(&*mut);\n"
+              "}\n", nullptr, false, false);
+        ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on mut - it's already a pointer.\n", errout.str());
+
+        // make sure we got the AST match for "(" right
+        check("void f(char *ptr) {\n"
+              "    if (&*ptr == NULL)\n"
+              "        return;\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on ptr - it's already a pointer.\n", errout.str());
+
+        // no warning for macros
+        check("#define MUTEX_LOCK(m) pthread_mutex_lock(&(m))\n"
+              "void f(struct mutex *mut) {\n"
+              "    MUTEX_LOCK(*mut);\n"
+              "}\n", nullptr, false, true);
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)