From c2d5aef4354dca33fd0e9ede40a6a1ea6f688a7a Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Tue, 31 Oct 2023 16:12:38 +0100 Subject: [PATCH] Fix #12083 FN passedByValue with usage in ternary (#5575) --- lib/checkother.cpp | 104 ++------------------------------------------- lib/path.cpp | 2 +- lib/path.h | 2 +- test/cfg/std.cpp | 2 +- test/testother.cpp | 15 +++++-- 5 files changed, 19 insertions(+), 106 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index e8c904976..8324fb130 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1227,104 +1227,6 @@ static int estimateSize(const Type* type, const Settings* settings, const Symbol }); } -static bool isConstRangeBasedFor(const Token* tok) { - if (astIsRangeBasedForDecl(tok)) { - const Variable* loopVar = tok->astParent()->astOperand1()->variable(); - return loopVar && (!loopVar->isReference() || loopVar->isConst()); - } - return false; -} - -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 && func_scope->type == Function::Type::eConstructor) { - //could be initialized in initializer list - const Token* init = func_scope->arg->link()->next(); - if (init->str() == "noexcept") { - init = init->next(); - if (init->link()) - init = init->link()->next(); - } - if (init->str() == ":") { - for (const Token* tok2 = func_scope->arg->link()->next()->next(); tok2 != var->scope()->bodyStart; tok2 = tok2->next()) { - if (tok2->varId() != var->declarationId()) - continue; - const Token* parent = tok2->astParent(); - if (parent && Token::simpleMatch(parent->previous(), "move (")) - return false; - } - } - } - } - for (const Token* tok2 = var->scope()->bodyStart; tok2 != var->scope()->bodyEnd; tok2 = tok2->next()) { - if (tok2->varId() != var->declarationId()) - continue; - - const Token* parent = tok2->astParent(); - while (Token::simpleMatch(parent, "[")) - parent = parent->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; - if (parent->str() == ">>" && parent->astOperand2() == tok2) - return false; - } else if (parent->str() == "," || parent->str() == "(") { // function argument - int argNr = -1; - const Token* functionTok = getTokenArgumentFunction(tok2, argNr); - if (!functionTok) - return false; - const Function* tokFunction = functionTok->function(); - if (tokFunction) { - const Variable* argVar = tokFunction->getArgumentVar(argNr); - if (!argVar || (!argVar->isConst() && argVar->isReference())) - return false; - } - else if (!settings->library.isFunctionConst(functionTok)) - return false; - } else if (parent->isUnaryOp("&")) { - // TODO: check how pointer is used - return false; - } else if (parent->isConstOp() || - (parent->astOperand2() && settings->library.isFunctionConst(parent->astOperand2()))) - continue; - else if (parent->isAssignmentOp()) { - const Token* assignee = parent->astOperand1(); - while (Token::simpleMatch(assignee, "[")) - assignee = assignee->astOperand1(); - if (assignee == tok2) - return false; - const Variable* assignedVar = assignee ? assignee->variable() : nullptr; - if (assignedVar && - !assignedVar->isConst() && - assignedVar->isReference() && - assignedVar->nameToken() == parent->astOperand1()) - return false; - } else if (Token::Match(tok2, "%var% . %name% (")) { - const Function* func = tok2->tokAt(2)->function(); - if (func && (func->isConst() || func->isStatic())) - continue; - return false; - } else if (isConstRangeBasedFor(tok2)) - continue; - else - return false; - } - - return true; -} - void CheckOther::checkPassByReference() { if (!mSettings->severity.isEnabled(Severity::performance) || mTokenizer->isC()) @@ -1374,7 +1276,7 @@ void CheckOther::checkPassByReference() if (!var->scope() || var->scope()->function->isImplicitlyVirtual()) continue; - if (canBeConst(var, mSettings)) { + if (!isVariableChanged(var, mSettings, mTokenizer->isCPP())) { passedByValueError(var, inconclusive); } } @@ -2939,7 +2841,9 @@ void CheckOther::checkRedundantCopy() const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Variable* var : symbolDatabase->variableList()) { - 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 + if (!var || var->isReference() || var->isPointer() || + (!var->type() && !var->isStlType()) || // bailout if var is of standard type, if it is a pointer or non-const + (!var->isConst() && isVariableChanged(var, mSettings, mTokenizer->isCPP()))) continue; const Token* startTok = var->nameToken(); diff --git a/lib/path.cpp b/lib/path.cpp index 16e366fec..1c1c724d3 100644 --- a/lib/path.cpp +++ b/lib/path.cpp @@ -284,7 +284,7 @@ bool Path::isDirectory(const std::string &path) return file_type(path) == S_IFDIR; } -std::string Path::join(std::string path1, std::string path2) { +std::string Path::join(const std::string& path1, const std::string& path2) { if (path1.empty() || path2.empty()) return path1 + path2; if (path2.front() == '/') diff --git a/lib/path.h b/lib/path.h index 6780e5e97..82545f44e 100644 --- a/lib/path.h +++ b/lib/path.h @@ -197,7 +197,7 @@ public: /** * join 2 paths with '/' separators */ - static std::string join(std::string path1, std::string path2); + static std::string join(const std::string& path1, const std::string& path2); }; /// @} diff --git a/test/cfg/std.cpp b/test/cfg/std.cpp index 81e5ba877..a281552c8 100644 --- a/test/cfg/std.cpp +++ b/test/cfg/std.cpp @@ -4497,7 +4497,7 @@ void getline() in.close(); } -// TODO cppcheck-suppress passedByValue +// cppcheck-suppress passedByValue void stream_write(std::ofstream& s, std::vector v) { if (v.empty()) {} s.write(v.data(), v.size()); diff --git a/test/testother.cpp b/test/testother.cpp index a5ac9521f..420014708 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2167,6 +2167,11 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:4]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str()); + check("void f(const std::string& s, std::string t) {\n" // #12083 + " const std::string& v = !s.empty() ? s : t;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 't' should be passed by const reference.\n", errout.str()); + Settings settings1 = settingsBuilder().platform(Platform::Type::Win64).build(); check("using ui64 = unsigned __int64;\n" "ui64 Test(ui64 one, ui64 two) { return one + two; }\n", @@ -2201,7 +2206,9 @@ private: check("void f(std::string str) {\n" " std::string& s2 = str;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Variable 's2' can be declared as reference to const\n", errout.str()); + ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n" + "[test.cpp:2]: (style) Variable 's2' can be declared as reference to const\n", + errout.str()); check("void f(std::string str) {\n" " const std::string& s2 = str;\n" @@ -2354,7 +2361,9 @@ private: " int& i = x[0];\n" " return i;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Variable 'i' can be declared as reference to const\n", errout.str()); + ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'x' should be passed by const reference.\n" + "[test.cpp:2]: (style) Variable 'i' can be declared as reference to const\n", + errout.str()); check("int f(std::vector& x) {\n" " return x[0];\n" @@ -2371,7 +2380,7 @@ private: " static int& i = x[0];\n" " return i;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'x' should be passed by const reference.\n", errout.str()); check("int f(std::vector x) {\n" " int& i = x[0];\n"