Skip to content

Commit 58cb6cc

Browse files
thomasjfoxthomasjfox
committed
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.
1 parent 2bcd675 commit 58cb6cc

3 files changed

Lines changed: 112 additions & 2 deletions

File tree

‎lib/checkother.cpp‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2853,3 +2853,40 @@ void CheckOther::ignoredReturnValueError(const Token* tok, const std::string& fu
28532853
reportError(tok, Severity::warning, "ignoredReturnValue",
28542854
"Return value of function " + function + "() is not used.", false);
28552855
}
2856+
2857+
void CheckOther::checkRedundantPointerOp()
2858+
{
2859+
if (!_settings->isEnabled("style"))
2860+
return;
2861+
2862+
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
2863+
for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
2864+
if (tok->str() == "&") {
2865+
// bail out for logical AND operator
2866+
if (tok->astOperand2())
2867+
continue;
2868+
2869+
// pointer dereference
2870+
const Token *astTok = tok->astOperand1();
2871+
if (!astTok || astTok->str() != "*")
2872+
continue;
2873+
2874+
// variable
2875+
const Token *varTok = astTok->astOperand1();
2876+
if (!varTok || varTok->isExpandedMacro() || varTok->varId() == 0)
2877+
continue;
2878+
2879+
const Variable *var = symbolDatabase->getVariableFromVarId(varTok->varId());
2880+
if (!var || !var->isPointer())
2881+
continue;
2882+
2883+
redundantPointerOpError(tok, var->name(), false);
2884+
}
2885+
}
2886+
}
2887+
2888+
void CheckOther::redundantPointerOpError(const Token* tok, const std::string &varname, bool inconclusive)
2889+
{
2890+
reportError(tok, Severity::style, "redundantPointerOp",
2891+
"Redundant pointer operation on " + varname + " - it's already a pointer.", inconclusive);
2892+
}

‎lib/checkother.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ class CPPCHECKLIB CheckOther : public Check {
7474
checkOther.checkNanInArithmeticExpression();
7575
checkOther.checkCommaSeparatedReturn();
7676
checkOther.checkIgnoredReturnValue();
77+
checkOther.checkRedundantPointerOp();
7778
}
7879

7980
/** @brief Run checks against the simplified token list */
@@ -233,6 +234,9 @@ class CPPCHECKLIB CheckOther : public Check {
233234
/** @brief %Check for ignored return values. */
234235
void checkIgnoredReturnValue();
235236

237+
/** @brief %Check for redundant pointer operations */
238+
void checkRedundantPointerOp();
239+
236240
private:
237241
// Error messages..
238242
void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result);
@@ -288,6 +292,7 @@ class CPPCHECKLIB CheckOther : public Check {
288292
void varFuncNullUBError(const Token *tok);
289293
void commaSeparatedReturnError(const Token *tok);
290294
void ignoredReturnValueError(const Token* tok, const std::string& function);
295+
void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive);
291296

292297
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
293298
CheckOther c(0, settings, errorLogger);
@@ -345,6 +350,7 @@ class CPPCHECKLIB CheckOther : public Check {
345350
c.nanInArithmeticExpressionError(0);
346351
c.commaSeparatedReturnError(0);
347352
c.ignoredReturnValueError(0, "malloc");
353+
c.redundantPointerOpError(0, "varname", false);
348354
}
349355

350356
static std::string myName() {
@@ -402,7 +408,8 @@ class CPPCHECKLIB CheckOther : public Check {
402408
"- NaN (not a number) value used in arithmetic expression.\n"
403409
"- comma in return statement (the comma can easily be misread as a semicolon).\n"
404410
"- prefer erfc, expm1 or log1p to avoid loss of precision.\n"
405-
"- identical code in both branches of if/else or ternary operator.\n";
411+
"- identical code in both branches of if/else or ternary operator.\n"
412+
"- redundant pointer operation on pointer like &*some_ptr.\n";
406413
}
407414
};
408415
/// @}

‎test/testother.cpp‎

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,11 @@ class TestOther : public TestFixture {
176176

177177
TEST_CASE(testReturnIgnoredReturnValue);
178178
TEST_CASE(testReturnIgnoredReturnValuePosix);
179+
180+
TEST_CASE(redundantPointerOp);
179181
}
180182

181-
void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool posix = false, bool runSimpleChecks=true, Settings* settings = 0) {
183+
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) {
182184
// Clear the error buffer..
183185
errout.str("");
184186

@@ -197,6 +199,14 @@ class TestOther : public TestFixture {
197199
settings->experimental = experimental;
198200
settings->standards.posix = posix;
199201

202+
// Preprocess file..
203+
Preprocessor preprocessor(settings);
204+
std::list<std::string> configurations;
205+
std::string filedata = "";
206+
std::istringstream fin(raw_code);
207+
preprocessor.preprocess(fin, filedata, configurations, "", settings->_includePaths);
208+
const std::string code = preprocessor.getcode(filedata, "", "");
209+
200210
// Tokenize..
201211
Tokenizer tokenizer(settings, this);
202212
std::istringstream istr(code);
@@ -6455,6 +6465,62 @@ class TestOther : public TestFixture {
64556465
);
64566466
ASSERT_EQUALS("[test.cpp:3]: (warning) Return value of function mmap() is not used.\n", errout.str());
64576467
}
6468+
6469+
void redundantPointerOp() {
6470+
check("int *f(int *x) {\n"
6471+
" return &*x;\n"
6472+
"}\n", nullptr, false, true);
6473+
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on x - it's already a pointer.\n", errout.str());
6474+
6475+
check("int *f(int *y) {\n"
6476+
" return &(*y);\n"
6477+
"}\n", nullptr, false, true);
6478+
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on y - it's already a pointer.\n", errout.str());
6479+
6480+
// no warning for bitwise AND
6481+
check("void f(int *b) {\n"
6482+
" int x = 0x20 & *b;\n"
6483+
"}\n", nullptr, false, true);
6484+
ASSERT_EQUALS("", errout.str());
6485+
6486+
// No message for double pointers to structs
6487+
check("void f(struct foo **my_struct) {\n"
6488+
" char **pass_to_func = &(*my_struct)->buf;\n"
6489+
"}\n", nullptr, false, true);
6490+
ASSERT_EQUALS("", errout.str());
6491+
6492+
// another double pointer to struct - with an array
6493+
check("void f(struct foo **my_struct) {\n"
6494+
" char **pass_to_func = &(*my_struct)->buf[10];\n"
6495+
"}\n", nullptr, false, true);
6496+
ASSERT_EQUALS("", errout.str());
6497+
6498+
// double pointer to array
6499+
check("void f(char **ptr) {\n"
6500+
" int *x = &(*ptr)[10];\n"
6501+
"}\n", nullptr, false, true);
6502+
ASSERT_EQUALS("", errout.str());
6503+
6504+
// function calls
6505+
check("void f(Mutex *mut) {\n"
6506+
" pthread_mutex_lock(&*mut);\n"
6507+
"}\n", nullptr, false, false);
6508+
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on mut - it's already a pointer.\n", errout.str());
6509+
6510+
// make sure we got the AST match for "(" right
6511+
check("void f(char *ptr) {\n"
6512+
" if (&*ptr == NULL)\n"
6513+
" return;\n"
6514+
"}\n", nullptr, false, true);
6515+
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant pointer operation on ptr - it's already a pointer.\n", errout.str());
6516+
6517+
// no warning for macros
6518+
check("#define MUTEX_LOCK(m) pthread_mutex_lock(&(m))\n"
6519+
"void f(struct mutex *mut) {\n"
6520+
" MUTEX_LOCK(*mut);\n"
6521+
"}\n", nullptr, false, true);
6522+
ASSERT_EQUALS("", errout.str());
6523+
}
64586524
};
64596525

64606526
REGISTER_TEST(TestOther)

0 commit comments

Comments
 (0)