diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 041b9d022cc..3e324a6833d 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1244,10 +1244,14 @@ void CheckOther::checkPassByReference() const SymbolDatabase * const symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Variable* var : symbolDatabase->variableList()) { - if (!var || !var->isArgument() || !var->isClass() || var->isPointer() || var->isArray() || var->isReference() || var->isEnumType()) + if (!var || !var->isClass() || var->isPointer() || var->isArray() || var->isReference() || var->isEnumType()) continue; - if (var->scope() && var->scope()->function->arg->link()->strAt(-1) == "...") + const bool isRangeBasedFor = astIsRangeBasedForDecl(var->nameToken()); + if (!var->isArgument() && !isRangeBasedFor) + continue; + + if (!isRangeBasedFor && var->scope() && var->scope()->function->arg->link()->strAt(-1) == "...") continue; // references could not be used as va_start parameters (#5824) const Token * const varDeclEndToken = var->declEndToken(); @@ -1275,25 +1279,27 @@ void CheckOther::checkPassByReference() const bool isConst = var->isConst(); if (isConst) { - passedByValueError(var, inconclusive); + passedByValueError(var, inconclusive, isRangeBasedFor); continue; } // Check if variable could be const - if (!var->scope() || var->scope()->function->isImplicitlyVirtual()) + if (!isRangeBasedFor && (!var->scope() || var->scope()->function->isImplicitlyVirtual())) continue; if (!isVariableChanged(var, mSettings, mTokenizer->isCPP())) { - passedByValueError(var, inconclusive); + passedByValueError(var, inconclusive, isRangeBasedFor); } } } -void CheckOther::passedByValueError(const Variable* var, bool inconclusive) +void CheckOther::passedByValueError(const Variable* var, bool inconclusive, bool isRangeBasedFor) { - std::string id = "passedByValue"; - std::string msg = "$symbol:" + (var ? var->name() : "") + "\n" - "Function parameter '$symbol' should be passed by const reference."; + std::string id = isRangeBasedFor ? "iterateByValue" : "passedByValue"; + const std::string action = isRangeBasedFor ? "declared as": "passed by"; + const std::string type = isRangeBasedFor ? "Range variable" : "Function parameter"; + std::string msg = "$symbol:" + (var ? var->name() : "") + "\n" + + type + " '$symbol' should be " + action + " const reference."; ErrorPath errorPath; if (var && var->scope() && var->scope()->function && var->scope()->function->functionPointerUsage) { id += "Callback"; @@ -1302,7 +1308,10 @@ void CheckOther::passedByValueError(const Variable* var, bool inconclusive) } if (var) errorPath.emplace_back(var->nameToken(), msg); - msg += "\nParameter '$symbol' is passed by value. It could be passed as a const reference which is usually faster and recommended in C++."; + if (isRangeBasedFor) + msg += "\nVariable '$symbol' is used to iterate by value. It could be declared as a const reference which is usually faster and recommended in C++."; + else + msg += "\nParameter '$symbol' is passed by value. It could be passed as a const reference which is usually faster and recommended in C++."; reportError(errorPath, Severity::performance, id.c_str(), msg, CWE398, inconclusive ? Certainty::inconclusive : Certainty::normal); } diff --git a/lib/checkother.h b/lib/checkother.h index 2499ef1b581..8f84007032a 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -241,7 +241,7 @@ class CPPCHECKLIB CheckOther : public Check { void clarifyStatementError(const Token* tok); void cstyleCastError(const Token *tok); void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive, bool toIsInt); - void passedByValueError(const Variable* var, bool inconclusive); + void passedByValueError(const Variable* var, bool inconclusive, bool isRangeBasedFor = false); void constVariableError(const Variable *var, const Function *function); void constStatementError(const Token *tok, const std::string &type, bool inconclusive); void signedCharArrayIndexError(const Token *tok); diff --git a/test/testother.cpp b/test/testother.cpp index a97f52b8dad..0c5f89a7469 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -289,6 +289,7 @@ class TestOther : public TestFixture { TEST_CASE(constVariableArrayMember); // #10371 TEST_CASE(knownPointerToBool); + TEST_CASE(iterateByValue); } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -11677,6 +11678,16 @@ class TestOther : public TestFixture { "}\n"); ASSERT_EQUALS("", errout.str()); } + + void iterateByValue() { + check("void f() {\n" // #9684 + " const std::set ss = { \"a\", \"b\", \"c\" };\n" + " for (auto s : ss)\n" + " (void)s.size();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (performance) Range variable 's' should be declared as const reference.\n", + errout.str()); + } }; REGISTER_TEST(TestOther)