Skip to content

add a performance checker for const assignment #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
WKPlus wants to merge 1 commit into from
Closed

add a performance checker for const assignment #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3004,3 +3004,51 @@ void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varn
"An unsigned variable '" + varname + "' can't be negative so it is unnecessary to test it.");
}
}
/*
This check rule works for checking the "const A a = getA()" usage when getA() returns "const A &" or "A &".
In most scenarios, "const A & a = getA()" will be more efficient.
*/
void CheckOther::checkRedundantCopy()
{
if (!_settings->isEnabled("performance"))
return;

const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();

for (const Token *tok = _tokenizer->tokens(); tok; tok=tok->next()) {
const char *expect_end_token;
if (Token::Match(tok, "const %type% %var% =")) {
//match "const A a =" usage
expect_end_token = ";";
} else if (Token::Match(tok, "const %type% %var% (")) {
//match "const A a (" usage
expect_end_token = ")";
} else {
continue;
}

if (tok->strAt(1) == tok->strAt(4)) //avoid "const A a = A();"
continue;
if (!symbolDatabase->isClassOrStruct(tok->next()->str())) //avoid when %type% is standard type
continue;
const Token *var_tok = tok->tokAt(2);
tok = tok->tokAt(4);
while (tok &&Token::Match(tok,"%var% ."))
tok = tok->tokAt(2);
if (!Token::Match(tok, "%var% ("))
break;
const Token *match_end = (tok->next()->link()!=NULL)?tok->next()->link()->next():NULL;
if (match_end==NULL || !Token::Match(match_end,expect_end_token)) //avoid usage like "const A a = getA()+3"
break;
const Token *fToken = _tokenizer->getFunctionTokenByName(tok->str().c_str());
if (fToken &&fToken->previous() && fToken->previous()->str() == "&") {
redundantCopyError(var_tok,var_tok->str());
}
}
}
void CheckOther::redundantCopyError(const Token *tok,const std::string& varname)
{
reportError(tok, Severity::performance,"redundantCopy",
"Const variable '"+varname+"' can be reference to avoid data copying.\n"
"Redundant copying of data. Consider changing const variable '"+varname+"' to const reference.");
}
10 changes: 10 additions & 0 deletions lib/checkother.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class CPPCHECKLIB CheckOther : public Check {
checkOther.checkAssignBoolToPointer();
checkOther.checkBitwiseOnBoolean();
checkOther.checkDoubleFree();
checkOther.checkRedundantCopy();
}

/** @brief Clarify calculation for ".. a * b ? .." */
Expand Down Expand Up @@ -235,6 +236,7 @@ class CPPCHECKLIB CheckOther : public Check {
/** @brief %Check for double free or double close operations */
void checkDoubleFree();
void doubleFreeError(const Token *tok, const std::string &varname);
void checkRedundantCopy();

private:
// Error messages..
Expand Down Expand Up @@ -290,6 +292,8 @@ class CPPCHECKLIB CheckOther : public Check {
void doubleCloseDirError(const Token *tok, const std::string &varname);
void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal);

void redundantCopyError(const Token *tok, const std::string &varname);

void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
CheckOther c(0, settings, errorLogger);

Expand All @@ -306,6 +310,9 @@ class CPPCHECKLIB CheckOther : public Check {
c.doubleFreeError(0, "varname");
c.invalidPointerCastError(0, "float", "double", false);

//performance
c.redundantCopyError(0, "varname");

// style/warning
c.cstyleCastError(0);
c.dangerousUsageStrtolError(0);
Expand Down Expand Up @@ -364,6 +371,9 @@ class CPPCHECKLIB CheckOther : public Check {
"* incorrect length arguments for 'substr' and 'strncmp'\n"
"* double free() or double closedir()\n"

//performance
"* redundant data copying for const variable\n"

// style
"* C-style pointer cast in cpp file\n"
"* casting between incompatible pointer types\n"
Expand Down
88 changes: 88 additions & 0 deletions test/testother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ class TestOther : public TestFixture {
TEST_CASE(checkForSuspiciousSemicolon2);

TEST_CASE(checkDoubleFree);

TEST_CASE(checkRedundantCopy);
}

void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true) {
Expand Down Expand Up @@ -5255,6 +5257,92 @@ class TestOther : public TestFixture {
);
ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
}

void check_redundant_copy(const char code[]) {
// Clear the error buffer..
errout.str("");

Settings settings;
settings.addEnabled("performance");

// Tokenize..
Tokenizer tokenizer(&settings, this);
std::istringstream istr(code);
tokenizer.tokenize(istr, "test.cpp");

// Simplify token list..
CheckOther checkOther(&tokenizer, &settings, this);
tokenizer.simplifyTokenList();
checkOther.checkRedundantCopy();
}
void checkRedundantCopy() {
check_redundant_copy("class A{public:A(){}};\n"
"const A& getA(){static A a;return a;}\n"
"int main()\n"
"{\n"
" const A a = getA();\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5]: (performance) Const variable 'a' can be reference to avoid data copying.\n", errout.str());

check_redundant_copy("const int& getA(){static int a;return a;}\n"
"int main()\n"
"{\n"
" const int a = getA();\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("", errout.str());

check_redundant_copy("const int& getA(){static int a;return a;}\n"
"int main()\n"
"{\n"
" int getA = 0;\n"
" const int a = getA + 3;\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("", errout.str());

check_redundant_copy("class A{public:A(){}};\n"
"const A& getA(){static A a;return a;}\n"
"int main()\n"
"{\n"
" const A a(getA());\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5]: (performance) Const variable 'a' can be reference to avoid data copying.\n", errout.str());

check_redundant_copy("const int& getA(){static int a;return a;}\n"
"int main()\n"
"{\n"
" const int a(getA());\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("", errout.str());

check_redundant_copy("class A{\n"
"public:A(int a=0){_a = a;}\n"
"A operator+(const A & a){return A(_a+a._a);}\n"
"private:int _a;};\n"
"const A& getA(){static A a;return a;}\n"
"int main()\n"
"{\n"
" const A a = getA() + 1;\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("", errout.str());

check_redundant_copy("class A{\n"
"public:A(int a=0){_a = a;}\n"
"A operator+(const A & a){return A(_a+a._a);}\n"
"private:int _a;};\n"
"const A& getA(){static A a;return a;}\n"
"int main()\n"
"{\n"
" const A a(getA()+1);\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
};

REGISTER_TEST(TestOther)