Skip to content

Commit c2d5aef

Browse files
chrchr-githubchrchr-github
authored
Fix #12083 FN passedByValue with usage in ternary (#5575)
1 parent bbaa7be commit c2d5aef

5 files changed

Lines changed: 19 additions & 106 deletions

File tree

‎lib/checkother.cpp‎

Lines changed: 4 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -1227,104 +1227,6 @@ static int estimateSize(const Type* type, const Settings* settings, const Symbol
12271227
});
12281228
}
12291229

1230-
static bool isConstRangeBasedFor(const Token* tok) {
1231-
if (astIsRangeBasedForDecl(tok)) {
1232-
const Variable* loopVar = tok->astParent()->astOperand1()->variable();
1233-
return loopVar && (!loopVar->isReference() || loopVar->isConst());
1234-
}
1235-
return false;
1236-
}
1237-
1238-
static bool canBeConst(const Variable *var, const Settings* settings)
1239-
{
1240-
if (!var->scope())
1241-
return false;
1242-
{
1243-
// check initializer list. If variable is moved from it can't be const.
1244-
const Function* func_scope = var->scope()->function;
1245-
if (func_scope && func_scope->type == Function::Type::eConstructor) {
1246-
//could be initialized in initializer list
1247-
const Token* init = func_scope->arg->link()->next();
1248-
if (init->str() == "noexcept") {
1249-
init = init->next();
1250-
if (init->link())
1251-
init = init->link()->next();
1252-
}
1253-
if (init->str() == ":") {
1254-
for (const Token* tok2 = func_scope->arg->link()->next()->next(); tok2 != var->scope()->bodyStart; tok2 = tok2->next()) {
1255-
if (tok2->varId() != var->declarationId())
1256-
continue;
1257-
const Token* parent = tok2->astParent();
1258-
if (parent && Token::simpleMatch(parent->previous(), "move ("))
1259-
return false;
1260-
}
1261-
}
1262-
}
1263-
}
1264-
for (const Token* tok2 = var->scope()->bodyStart; tok2 != var->scope()->bodyEnd; tok2 = tok2->next()) {
1265-
if (tok2->varId() != var->declarationId())
1266-
continue;
1267-
1268-
const Token* parent = tok2->astParent();
1269-
while (Token::simpleMatch(parent, "["))
1270-
parent = parent->astParent();
1271-
if (!parent)
1272-
continue;
1273-
if (Token::simpleMatch(tok2->next(), ";") && tok2->next()->isSplittedVarDeclEq()) {
1274-
tok2 = tok2->tokAt(2);
1275-
tok2 = Token::findsimplematch(tok2, ";");
1276-
continue;
1277-
}
1278-
if (parent->str() == "<<" || isLikelyStreamRead(true, parent)) {
1279-
if (parent->str() == "<<" && parent->astOperand1() == tok2)
1280-
return false;
1281-
if (parent->str() == ">>" && parent->astOperand2() == tok2)
1282-
return false;
1283-
} else if (parent->str() == "," || parent->str() == "(") { // function argument
1284-
int argNr = -1;
1285-
const Token* functionTok = getTokenArgumentFunction(tok2, argNr);
1286-
if (!functionTok)
1287-
return false;
1288-
const Function* tokFunction = functionTok->function();
1289-
if (tokFunction) {
1290-
const Variable* argVar = tokFunction->getArgumentVar(argNr);
1291-
if (!argVar || (!argVar->isConst() && argVar->isReference()))
1292-
return false;
1293-
}
1294-
else if (!settings->library.isFunctionConst(functionTok))
1295-
return false;
1296-
} else if (parent->isUnaryOp("&")) {
1297-
// TODO: check how pointer is used
1298-
return false;
1299-
} else if (parent->isConstOp() ||
1300-
(parent->astOperand2() && settings->library.isFunctionConst(parent->astOperand2())))
1301-
continue;
1302-
else if (parent->isAssignmentOp()) {
1303-
const Token* assignee = parent->astOperand1();
1304-
while (Token::simpleMatch(assignee, "["))
1305-
assignee = assignee->astOperand1();
1306-
if (assignee == tok2)
1307-
return false;
1308-
const Variable* assignedVar = assignee ? assignee->variable() : nullptr;
1309-
if (assignedVar &&
1310-
!assignedVar->isConst() &&
1311-
assignedVar->isReference() &&
1312-
assignedVar->nameToken() == parent->astOperand1())
1313-
return false;
1314-
} else if (Token::Match(tok2, "%var% . %name% (")) {
1315-
const Function* func = tok2->tokAt(2)->function();
1316-
if (func && (func->isConst() || func->isStatic()))
1317-
continue;
1318-
return false;
1319-
} else if (isConstRangeBasedFor(tok2))
1320-
continue;
1321-
else
1322-
return false;
1323-
}
1324-
1325-
return true;
1326-
}
1327-
13281230
void CheckOther::checkPassByReference()
13291231
{
13301232
if (!mSettings->severity.isEnabled(Severity::performance) || mTokenizer->isC())
@@ -1374,7 +1276,7 @@ void CheckOther::checkPassByReference()
13741276
if (!var->scope() || var->scope()->function->isImplicitlyVirtual())
13751277
continue;
13761278

1377-
if (canBeConst(var, mSettings)) {
1279+
if (!isVariableChanged(var, mSettings, mTokenizer->isCPP())) {
13781280
passedByValueError(var, inconclusive);
13791281
}
13801282
}
@@ -2939,7 +2841,9 @@ void CheckOther::checkRedundantCopy()
29392841
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
29402842

29412843
for (const Variable* var : symbolDatabase->variableList()) {
2942-
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
2844+
if (!var || var->isReference() || var->isPointer() ||
2845+
(!var->type() && !var->isStlType()) || // bailout if var is of standard type, if it is a pointer or non-const
2846+
(!var->isConst() && isVariableChanged(var, mSettings, mTokenizer->isCPP())))
29432847
continue;
29442848

29452849
const Token* startTok = var->nameToken();

‎lib/path.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ bool Path::isDirectory(const std::string &path)
284284
return file_type(path) == S_IFDIR;
285285
}
286286

287-
std::string Path::join(std::string path1, std::string path2) {
287+
std::string Path::join(const std::string& path1, const std::string& path2) {
288288
if (path1.empty() || path2.empty())
289289
return path1 + path2;
290290
if (path2.front() == '/')

‎lib/path.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ class CPPCHECKLIB Path {
197197
/**
198198
* join 2 paths with '/' separators
199199
*/
200-
static std::string join(std::string path1, std::string path2);
200+
static std::string join(const std::string& path1, const std::string& path2);
201201
};
202202

203203
/// @}

‎test/cfg/std.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4497,7 +4497,7 @@ void getline()
44974497
in.close();
44984498
}
44994499

4500-
// TODO cppcheck-suppress passedByValue
4500+
// cppcheck-suppress passedByValue
45014501
void stream_write(std::ofstream& s, std::vector<char> v) {
45024502
if (v.empty()) {}
45034503
s.write(v.data(), v.size());

‎test/testother.cpp‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2167,6 +2167,11 @@ class TestOther : public TestFixture {
21672167
"}\n");
21682168
ASSERT_EQUALS("[test.cpp:4]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
21692169

2170+
check("void f(const std::string& s, std::string t) {\n" // #12083
2171+
" const std::string& v = !s.empty() ? s : t;\n"
2172+
"}\n");
2173+
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 't' should be passed by const reference.\n", errout.str());
2174+
21702175
Settings settings1 = settingsBuilder().platform(Platform::Type::Win64).build();
21712176
check("using ui64 = unsigned __int64;\n"
21722177
"ui64 Test(ui64 one, ui64 two) { return one + two; }\n",
@@ -2201,7 +2206,9 @@ class TestOther : public TestFixture {
22012206
check("void f(std::string str) {\n"
22022207
" std::string& s2 = str;\n"
22032208
"}");
2204-
ASSERT_EQUALS("[test.cpp:2]: (style) Variable 's2' can be declared as reference to const\n", errout.str());
2209+
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n"
2210+
"[test.cpp:2]: (style) Variable 's2' can be declared as reference to const\n",
2211+
errout.str());
22052212

22062213
check("void f(std::string str) {\n"
22072214
" const std::string& s2 = str;\n"
@@ -2354,7 +2361,9 @@ class TestOther : public TestFixture {
23542361
" int& i = x[0];\n"
23552362
" return i;\n"
23562363
"}");
2357-
ASSERT_EQUALS("[test.cpp:2]: (style) Variable 'i' can be declared as reference to const\n", errout.str());
2364+
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'x' should be passed by const reference.\n"
2365+
"[test.cpp:2]: (style) Variable 'i' can be declared as reference to const\n",
2366+
errout.str());
23582367

23592368
check("int f(std::vector<int>& x) {\n"
23602369
" return x[0];\n"
@@ -2371,7 +2380,7 @@ class TestOther : public TestFixture {
23712380
" static int& i = x[0];\n"
23722381
" return i;\n"
23732382
"}");
2374-
ASSERT_EQUALS("", errout.str());
2383+
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'x' should be passed by const reference.\n", errout.str());
23752384

23762385
check("int f(std::vector<int> x) {\n"
23772386
" int& i = x[0];\n"

0 commit comments

Comments
 (0)