From 58cb6cc11617d26a788ce82d5fcacbe0b7cdea44 Mon Sep 17 00:00:00 2001 From: Thomas Jarosch Date: Sat, 17 Jan 2015 14:33:14 +0100 Subject: [PATCH] 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 | 37 +++++++++++++++++++++++++ lib/checkother.h | 9 +++++- test/testother.cpp | 68 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 112 insertions(+), 2 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index d082993d7..7df3690de 100644 --- a/lib/checkother.cpp +++ b/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); +} diff --git a/lib/checkother.h b/lib/checkother.h index ff5647ed5..9665f4227 100644 --- a/lib/checkother.h +++ b/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"; } }; /// @} diff --git a/test/testother.cpp b/test/testother.cpp index 6df5e23c7..8e55a4bed 100644 --- a/test/testother.cpp +++ b/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 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)