Fix #9684 New check: find unnecessary copy in range loop (#5738)

Author: chrchr-github

gui/checkthread.cpp

@@ -177,7 +177,7 @@ void CheckThread::runAddonsAndTools(const FileSettings *fileSettings, const QStr
             const QString clangPath = CheckThread::clangTidyCmd();
             if (!clangPath.isEmpty()) {
                 QDir dir(clangPath + "/../lib/clang");
-                for (QString ver : dir.entryList()) {
+                for (const QString& ver : dir.entryList()) {
                     QString includePath = dir.absolutePath() + '/' + ver + "/include";
                     if (ver[0] != '.' && QDir(includePath).exists()) {
                         args << "-isystem" << includePath;

gui/cppchecklibrarydata.cpp

@@ -697,14 +697,14 @@ static void writeFunction(QXmlStreamWriter &xmlWriter, const CppcheckLibraryData
     }
     if (!function.notOverlappingDataArgs.isEmpty()) {
         xmlWriter.writeStartElement("not-overlapping-data");
-        foreach (const QString value, function.notOverlappingDataArgs) {
+        foreach (const QString& value, function.notOverlappingDataArgs) {
             xmlWriter.writeAttribute(function.notOverlappingDataArgs.key(value), value);
         }
         xmlWriter.writeEndElement();
     }
     if (!function.containerAttributes.isEmpty()) {
         xmlWriter.writeStartElement("container");
-        foreach (const QString value, function.containerAttributes) {
+        foreach (const QString& value, function.containerAttributes) {
             xmlWriter.writeAttribute(function.containerAttributes.key(value), value);
         }
         xmlWriter.writeEndElement();

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);
 }
 

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);

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__)
@@ -11707,6 +11708,16 @@ class TestOther : public TestFixture {
               "}\n");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void iterateByValue() {
+        check("void f() {\n" // #9684
+              "    const std::set<std::string> 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)