From f5c4a21eaefb0eef46b1100b5c1bd504e2ae994b Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Sun, 10 Jul 2022 11:33:24 +0200 Subject: [PATCH] Fix #10704 FN redundantCopyLocalConst (#4115) --- lib/astutils.cpp | 3 ++- lib/checkother.cpp | 29 ++++++++++++++++++++++++----- lib/symboldatabase.cpp | 4 ++++ lib/symboldatabase.h | 6 ++++++ lib/templatesimplifier.cpp | 1 - lib/tokenize.cpp | 4 ++-- lib/valueflow.cpp | 8 ++++---- test/testother.cpp | 36 +++++++++++++++++++++++++++++++++--- 8 files changed, 75 insertions(+), 16 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index dc1f6ac7a..e7c693f10 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2615,7 +2615,8 @@ bool isExpressionChanged(const Token* expr, const Token* start, const Token* end if (tok->variable()) { if (tok->variable()->isConst()) return false; - global = !tok->variable()->isLocal() && !tok->variable()->isArgument(); + global = !tok->variable()->isLocal() && !tok->variable()->isArgument() && + !(tok->variable()->isMember() && !tok->variable()->isStatic()); } else if (tok->isIncompleteVar() && !tok->isIncompleteConstant()) { global = true; } diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 5f77ff319..65d2796b3 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1181,10 +1181,12 @@ static int estimateSize(const Type* type, const Settings* settings, const Symbol static bool canBeConst(const Variable *var, const Settings* settings) { + if (!var->scope()) + return false; { // check initializer list. If variable is moved from it can't be const. const Function* func_scope = var->scope()->function; - if (func_scope->type == Function::Type::eConstructor) { + if (func_scope && func_scope->type == Function::Type::eConstructor) { //could be initialized in initializer list if (func_scope->arg->link()->next()->str() == ":") { for (const Token* tok2 = func_scope->arg->link()->next()->next(); tok2 != var->scope()->bodyStart; tok2 = tok2->next()) { @@ -1204,6 +1206,11 @@ static bool canBeConst(const Variable *var, const Settings* settings) const Token* parent = tok2->astParent(); if (!parent) continue; + if (Token::simpleMatch(tok2->next(), ";") && tok2->next()->isSplittedVarDeclEq()) { + tok2 = tok2->tokAt(2); + tok2 = Token::findsimplematch(tok2, ";"); + continue; + } if (parent->str() == "<<" || isLikelyStreamRead(true, parent)) { if (parent->str() == "<<" && parent->astOperand1() == tok2) return false; @@ -2701,7 +2708,7 @@ void CheckOther::checkRedundantCopy() const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Variable* var : symbolDatabase->variableList()) { - if (!var || var->isReference() || !var->isConst() || var->isPointer() || (!var->type() && !var->isStlType())) // bailout if var is of standard type, if it is a pointer or non-const + if (!var || var->isReference() || (!var->isConst() && !canBeConst(var, mSettings)) || var->isPointer() || (!var->type() && !var->isStlType())) // bailout if var is of standard type, if it is a pointer or non-const continue; const Token* startTok = var->nameToken(); @@ -2711,6 +2718,8 @@ void CheckOther::checkRedundantCopy() // Object is instantiated. Warn if constructor takes arguments by value. if (constructorTakesReference(var->typeScope())) continue; + } else if (Token::simpleMatch(startTok->next(), ";") && startTok->next()->isSplittedVarDeclEq()) { + startTok = startTok->tokAt(2); } else continue; @@ -2723,17 +2732,27 @@ void CheckOther::checkRedundantCopy() continue; const Token* dot = tok->astOperand1(); - if (Token::simpleMatch(dot, ".") && dot->astOperand1() && isVariableChanged(dot->astOperand1()->variable(), mSettings, mTokenizer->isCPP())) - continue; + if (Token::simpleMatch(dot, ".")) { + if (dot->astOperand1() && isVariableChanged(dot->astOperand1()->variable(), mSettings, mTokenizer->isCPP())) + continue; + if (isTemporary(/*cpp*/ true, dot, &mSettings->library, /*unknown*/ true)) + continue; + } if (exprDependsOnThis(tok->previous())) continue; const Function* func = tok->previous()->function(); if (func && func->tokenDef->strAt(-1) == "&") { - redundantCopyError(startTok, startTok->str()); + const Scope* fScope = func->functionScope; + if (fScope && fScope->bodyEnd && Token::Match(fScope->bodyEnd->tokAt(-3), "return %var% ;")) { + const Token* varTok = fScope->bodyEnd->tokAt(-2); + if (varTok->variable() && !varTok->variable()->isGlobal()) + redundantCopyError(startTok, startTok->str()); + } } } } + void CheckOther::redundantCopyError(const Token *tok,const std::string& varname) { reportError(tok, Severity::performance, "redundantCopyLocalConst", diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 00c0b2005..a877fbd2b 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -2091,6 +2091,10 @@ Variable& Variable::operator=(const Variable &var) return *this; } +bool Variable::isMember() const { + return mScope && mScope->isClassOrStructOrUnion(); +} + bool Variable::isPointerArray() const { return isArray() && nameToken() && nameToken()->previous() && (nameToken()->previous()->str() == "*"); diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 18a99b12e..ec089f5df 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -379,6 +379,12 @@ public: return (mAccess == AccessControl::Local) && !isExtern(); } + /** + * Is variable a member of a user-defined type. + * @return true if member, false if not or unknown + */ + bool isMember() const; + /** * Is variable mutable. * @return true if mutable, false if not diff --git a/lib/templatesimplifier.cpp b/lib/templatesimplifier.cpp index bffb216bd..4d38c4cfa 100644 --- a/lib/templatesimplifier.cpp +++ b/lib/templatesimplifier.cpp @@ -3899,7 +3899,6 @@ void TemplateSimplifier::simplifyTemplates( continue; } - // cppcheck-suppress redundantCopyLocalConst ; False positive const std::string strop = op->str(); const std::string strargs = (args && args->isName()) ? args->str() : ""; diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 2ee5b6b3b..8f3003a6d 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -2334,7 +2334,7 @@ bool Tokenizer::simplifyUsing() Token::Match(tok->linkAt(2), "] ] = ::| %name%"))))) continue; - std::string name = tok->strAt(1); + const std::string& name = tok->strAt(1); const Token *nameToken = tok->next(); std::string scope = currentScope->fullName; Token *usingStart = tok; @@ -9518,7 +9518,7 @@ void Tokenizer::simplifyOperatorName() for (Token *tok = list.front(); tok; tok = tok->next()) { if (Token::Match(tok, "%op% %str% %name%")) { - std::string name = tok->strAt(2); + const std::string name = tok->strAt(2); Token * const str = tok->next(); str->deleteNext(); tok->insertToken("operator\"\"" + name); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 0ab6c3fca..edc31e553 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5596,7 +5596,7 @@ static void valueFlowAfterSwap(TokenList* tokenlist, continue; for (int i = 0; i < 2; i++) { std::vector vars = getVariables(args[0]); - std::list values = args[0]->values(); + const std::list& values = args[0]->values(); valueFlowForwardAssign(args[0], args[1], vars, values, false, tokenlist, errorLogger, settings); std::swap(args[0], args[1]); } @@ -6190,7 +6190,7 @@ struct ConditionHandler { parent = parent->astParent(); bool possible = false; if (Token::Match(parent, "&&|%oror%")) { - std::string op = parent->str(); + const std::string& op(parent->str()); while (parent && parent->str() == op) parent = parent->astParent(); if (Token::simpleMatch(parent, "!") || Token::simpleMatch(parent, "== false")) @@ -7732,7 +7732,7 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge if (Token::Match(tok, "%var% (|{") && tok->next()->astOperand2() && tok->next()->astOperand2()->str() != ",") { Token* inTok = tok->next()->astOperand2(); - std::list values = inTok->values(); + const std::list& values = inTok->values(); const bool constValue = inTok->isNumber(); valueFlowForwardAssign(inTok, var, values, constValue, true, tokenlist, errorLogger, settings); @@ -7760,7 +7760,7 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge Token* inTok = ftok->astOperand2(); if (!inTok) continue; - std::list values = inTok->values(); + const std::list& values = inTok->values(); valueFlowForwardAssign(inTok, tok, vars, values, false, tokenlist, errorLogger, settings); } } else if (Token::simpleMatch(tok->astParent(), ". release ( )")) { diff --git a/test/testother.cpp b/test/testother.cpp index 5f81695cd..50c3011c7 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -6657,7 +6657,8 @@ private: " if (i == j) {}\n" "}"); ASSERT_EQUALS( - "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n", + "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n" + "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:6]: (style) The comparison 'i == j' is always true because 'i' and 'j' represent the same value.\n", errout.str()); check("struct A { int x; int y; };" @@ -6669,7 +6670,8 @@ private: " if (i == a.x) {}\n" "}"); ASSERT_EQUALS( - "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n", + "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n" + "[test.cpp:3] -> [test.cpp:6]: (style) The comparison 'i == a.x' is always true because 'i' and 'a.x' represent the same value.\n", errout.str()); check("struct A { int x; int y; };" @@ -6681,7 +6683,8 @@ private: " if (j == a.x) {}\n" "}"); ASSERT_EQUALS( - "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n", + "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n" + "[test.cpp:4] -> [test.cpp:6]: (style) The comparison 'j == a.x' is always true because 'j' and 'a.x' represent the same value.\n", errout.str()); // Issue #8612 @@ -7603,6 +7606,33 @@ private: " }\n" "};\n", nullptr, /*experimental*/ false, /*inconclusive*/ true); ASSERT_EQUALS("", errout.str()); + + // #10704 + check("struct C {\n" + " std::string str;\n" + " const std::string& get() const { return str; }\n" + "};\n" + "struct D {\n" + " C c;\n" + " bool f() const {\n" + " std::string s = c.get();\n" + " return s.empty();\n" + " }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:8]: (performance, inconclusive) Use const reference for 's' to avoid unnecessary data copying.\n", errout.str()); + + check("struct C {\n" + " const std::string & get() const { return m; }\n" + " std::string m;\n" + "};\n" + "C getC();\n" + "void f() {\n" + " const std::string s = getC().get();\n" + "}\n" + "void g() {\n" + " std::string s = getC().get();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void checkNegativeShift() {