mitigated and enabled more clang-tidy warnings (#4470)

* fixed some `performance-inefficient-string-concatenation` clang-tidy warnings

* fixed and enabled `modernize-replace-random-shuffle` clang-tidy warning

* fixed and enabled `bugprone-suspicious-string-compare` clang-tidy warning

* mitigated and enabled `readability-non-const-parameter` clang-tidy warnings

* clang-tidy.md: documented some disabled checks

* mitigated and enabled `performance-unnecessary-value-param` clang-tidy warnings

.clang-tidy

@@ -1,5 +1,5 @@
 ---
-Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-named-parameter,-readability-redundant-member-init,-performance-faster-string-find,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-readability-simplify-boolean-expr,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-performance-unnecessary-value-param,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-readability-non-const-parameter,-misc-non-private-member-variables-in-classes,-bugprone-suspicious-string-compare,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-bugprone-suspicious-include,-modernize-replace-random-shuffle,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-performance-noexcept-move-constructor,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics'
+Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-named-parameter,-readability-redundant-member-init,-performance-faster-string-find,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-readability-simplify-boolean-expr,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-bugprone-suspicious-include,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-performance-noexcept-move-constructor,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics'
 WarningsAsErrors: '*'
 CheckOptions:
   - key:             misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic

clang-tidy.md

@@ -110,24 +110,29 @@ Work in progress.
 
 Is reported for valid patterns we are using.
 
+`readability-suspicious-call-argument`<br>
+
+Produces a lot of false positives since it is too vague in its analysis.
+
+`performance-inefficient-string-concatenation`<br>
+
+Produces many warnings which very much look like false ppsitives (needs to be reported upstream).
+
+`bugprone-suspicious-include`<br>
+
+Causes warnings with `*.cpp` includes in Qt generated files.
+
 `modernize-avoid-c-arrays`<br>
 `readability-container-size-empty`<br>
 `bugprone-branch-clone`<br>
 `readability-const-return-type`<br>
-`performance-unnecessary-value-param`<br>
 `modernize-return-braced-init-list`<br>
-`performance-inefficient-string-concatenation`<br>
 `misc-throw-by-value-catch-by-reference`<br>
 `readability-avoid-const-params-in-decls`<br>
-`readability-non-const-parameter`<br>
-`bugprone-suspicious-string-compare`<br>
 `bugprone-signed-char-misuse`<br>
-`bugprone-suspicious-include`<br>
-`modernize-replace-random-shuffle`<br>
 `readability-redundant-access-specifiers`<br>
 `performance-noexcept-move-constructor`<br>
 `concurrency-mt-unsafe`<br>
-`readability-suspicious-call-argument`<br>
 
 To be evaluated.
 

cli/cppcheckexecutor.cpp

@@ -575,8 +575,9 @@ bool CppCheckExecutor::tryLoadLibrary(Library& destination, const std::string& b
 /**
  * Execute a shell command and read the output from it. Returns true if command terminated successfully.
  */
-// cppcheck-suppress passedByValue - "exe" copy needed in _WIN32 code
-bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string> args, const std::string &redirect, std::string *output_)
+// cppcheck-suppress passedByValue - used as callback so we need to preserve the signature
+// NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
+bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output_)
 {
     output_->clear();
 

cli/cppcheckexecutor.h

@@ -114,7 +114,7 @@ public:
     /**
      * Execute a shell command and read the output from it. Returns true if command terminated successfully.
      */
-    static bool executeCommand(std::string exe, std::vector<std::string> args, const std::string &redirect, std::string *output_);
+    static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output_);
 
     static bool reportSuppressions(const Settings &settings, bool unusedFunctionCheckEnabled, const std::map<std::string, std::size_t> &files, ErrorLogger& errorLogger);
 

gui/checkthread.cpp

@@ -44,6 +44,7 @@
 #include <QRegularExpression>
 #include <QSettings>
 
+// NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
 static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output)
 {
     output->clear();

gui/test/cppchecklibrarydata/testcppchecklibrarydata.cpp

@@ -589,7 +589,7 @@ void TestCppcheckLibraryData::containerValid()
     }
 }
 
-void TestCppcheckLibraryData::loadCfgFile(QString filename, CppcheckLibraryData &data, QString &res, bool removeFile)
+void TestCppcheckLibraryData::loadCfgFile(const QString &filename, CppcheckLibraryData &data, QString &res, bool removeFile)
 {
     QFile file(filename);
     QVERIFY(file.open(QIODevice::ReadOnly | QIODevice::Text));
@@ -600,7 +600,7 @@ void TestCppcheckLibraryData::loadCfgFile(QString filename, CppcheckLibraryData
     }
 }
 
-void TestCppcheckLibraryData::saveCfgFile(QString filename, CppcheckLibraryData &data)
+void TestCppcheckLibraryData::saveCfgFile(const QString &filename, CppcheckLibraryData &data)
 {
     QFile file(filename);
     QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text));

gui/test/cppchecklibrarydata/testcppchecklibrarydata.h

@@ -42,8 +42,8 @@ private slots:
     void containerValid();
 
 private:
-    static void loadCfgFile(QString filename, CppcheckLibraryData &data, QString &res, bool removeFile = false);
-    static void saveCfgFile(QString filename, CppcheckLibraryData &data);
+    static void loadCfgFile(const QString &filename, CppcheckLibraryData &data, QString &res, bool removeFile = false);
+    static void saveCfgFile(const QString &filename, CppcheckLibraryData &data);
 
     CppcheckLibraryData libraryData;
     CppcheckLibraryData fileLibraryData;

lib/astutils.cpp

@@ -1260,7 +1260,7 @@ static bool isSameLifetime(const Token * const tok1, const Token * const tok2)
     return v1.tokvalue == v2.tokvalue;
 }
 
-static bool compareKnownValue(const Token * const tok1, const Token * const tok2, std::function<bool(const ValueFlow::Value&, const ValueFlow::Value&, bool)> compare)
+static bool compareKnownValue(const Token * const tok1, const Token * const tok2, const std::function<bool(const ValueFlow::Value&, const ValueFlow::Value&, bool)> &compare)
 {
     static const auto isKnownFn = std::mem_fn(&ValueFlow::Value::isKnown);
 

lib/checknullpointer.cpp

@@ -551,6 +551,7 @@ std::string CheckNullPointer::MyFileInfo::toString() const
     return CTU::toString(unsafeUsage);
 }
 
+// NOLINTNEXTLINE(readability-non-const-parameter) - used as callback so we need to preserve the signature
 static bool isUnsafeUsage(const Check *check, const Token *vartok, MathLib::bigint *value)
 {
     (void)value;

lib/checkuninitvar.cpp

@@ -1659,6 +1659,7 @@ Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const S
     return checker.getFileInfo();
 }
 
+// NOLINTNEXTLINE(readability-non-const-parameter) - used as callback so we need to preserve the signature
 static bool isVariableUsage(const Check *check, const Token *vartok, MathLib::bigint *value)
 {
     (void)value;

lib/clangimport.cpp

@@ -638,7 +638,7 @@ void clangimport::AstNode::setValueType(Token *tok)
 
 Scope *clangimport::AstNode::createScope(TokenList *tokenList, Scope::ScopeType scopeType, AstNodePtr astNode, const Token *def)
 {
-    std::vector<AstNodePtr> children2{astNode};
+    std::vector<AstNodePtr> children2{std::move(astNode)};
     return createScope(tokenList, scopeType, children2, def);
 }
 

lib/cppcheck.cpp

@@ -276,7 +276,7 @@ static std::string executeAddon(const AddonInfo &addonInfo,
                                 const std::string &defaultPythonExe,
                                 const std::string &file,
                                 const std::string &premiumArgs,
-                                std::function<bool(std::string,std::vector<std::string>,std::string,std::string*)> executeCommand)
+                                const std::function<bool(std::string,std::vector<std::string>,std::string,std::string*)> &executeCommand)
 {
     const std::string redirect = "2>&1";
 
@@ -377,7 +377,7 @@ const char * CppCheck::extraVersion()
     return ExtraVersion;
 }
 
-static bool reportClangErrors(std::istream &is, std::function<void(const ErrorMessage&)> reportErr, std::vector<ErrorMessage> *warnings)
+static bool reportClangErrors(std::istream &is, const std::function<void(const ErrorMessage&)>& reportErr, std::vector<ErrorMessage> *warnings)
 {
     std::string line;
     while (std::getline(is, line)) {

lib/token.cpp

@@ -627,7 +627,7 @@ bool Token::simpleMatch(const Token *tok, const char pattern[], size_t pattern_l
     while (*current) {
         const std::size_t length = next - current;
 
-        if (!tok || length != tok->mStr.length() || std::strncmp(current, tok->mStr.c_str(), length))
+        if (!tok || length != tok->mStr.length() || std::strncmp(current, tok->mStr.c_str(), length) != 0)
             return false;
 
         current = next;
@@ -2307,7 +2307,7 @@ std::string Token::typeStr(const Token* tok)
 
 void Token::scopeInfo(std::shared_ptr<ScopeInfo2> newScopeInfo)
 {
-    mImpl->mScopeInfo = newScopeInfo;
+    mImpl->mScopeInfo = std::move(newScopeInfo);
 }
 std::shared_ptr<ScopeInfo2> Token::scopeInfo() const
 {

lib/tokenize.cpp

@@ -5471,7 +5471,7 @@ static std::string getExpression(const Token *tok)
         line = prev->str() + " " + line;
     line += "!!!" + tok->str() + "!!!";
     for (const Token *next = tok->next(); next && !Token::Match(next, "[;{}]"); next = next->next())
-        line = line + " " + next->str();
+        line += " " + next->str();
     return line;
 }
 

lib/valueflow.cpp

@@ -1990,7 +1990,7 @@ static void valueFlowReverse(TokenList* tokenlist,
                              const Settings* = nullptr,
                              SourceLocation loc = SourceLocation::current())
 {
-    std::list<ValueFlow::Value> values = {val};
+    std::list<ValueFlow::Value> values = {std::move(val)};
     if (val2.varId != 0)
         values.push_back(val2);
     valueFlowReverse(tok, nullptr, varToken, values, tokenlist, loc);
@@ -4504,7 +4504,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
             };
 
             bool update = false;
-            auto captureVariable = [&](const Token* tok2, LifetimeCapture c, std::function<bool(const Token*)> pred) {
+            auto captureVariable = [&](const Token* tok2, LifetimeCapture c, const std::function<bool(const Token*)> &pred) {
                 if (varids.count(tok->varId()) > 0)
                     return;
                 if (c == LifetimeCapture::ByReference) {
@@ -6405,7 +6405,7 @@ ValueFlow::Value inferCondition(const std::string& op, const Token* varTok, Math
     return ValueFlow::Value{};
 }
 
-ValueFlow::Value inferCondition(std::string op, MathLib::bigint val, const Token* varTok)
+ValueFlow::Value inferCondition(const std::string &op, MathLib::bigint val, const Token* varTok)
 {
     if (!varTok)
         return ValueFlow::Value{};
@@ -8928,7 +8928,7 @@ std::string ValueFlow::eitherTheConditionIsRedundant(const Token *condition)
 
 const ValueFlow::Value* ValueFlow::findValue(const std::list<ValueFlow::Value>& values,
                                              const Settings* settings,
-                                             std::function<bool(const ValueFlow::Value&)> pred)
+                                             const std::function<bool(const ValueFlow::Value&)> &pred)
 {
     const ValueFlow::Value* ret = nullptr;
     for (const ValueFlow::Value& v : values) {

lib/valueflow.h

@@ -461,7 +461,7 @@ namespace ValueFlow {
 
     const ValueFlow::Value* findValue(const std::list<ValueFlow::Value>& values,
                                       const Settings* settings,
-                                      std::function<bool(const ValueFlow::Value&)> pred);
+                                      const std::function<bool(const ValueFlow::Value&)> &pred);
 
     std::vector<ValueFlow::Value> isOutOfBounds(const Value& size, const Token* indexTok, bool possible = true);
 }
@@ -502,7 +502,7 @@ struct LifetimeToken {
 const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, ValueFlow::Value &false_value, const std::function<std::vector<MathLib::bigint>(const Token*)>& evaluate);
 const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, ValueFlow::Value &false_value);
 
-ValueFlow::Value inferCondition(std::string op, MathLib::bigint val, const Token* varTok);
+ValueFlow::Value inferCondition(const std::string& op, MathLib::bigint val, const Token* varTok);
 ValueFlow::Value inferCondition(const std::string& op, const Token* varTok, MathLib::bigint val);
 
 CPPCHECKLIB ValuePtr<InferModel> makeIntegralInferModel();

test/testtokenize.cpp

@@ -5856,7 +5856,7 @@ private:
             if (tok->astOperand1() && astTop.find(tok->astTop()) == astTop.end()) {
                 astTop.insert(tok->astTop());
                 if (!ret.empty())
-                    ret = ret + " ";
+                    ret += " ";
                 ret += tok->astTop()->astString();
             }
         }

tools/triage/mainwindow.cpp

@@ -22,6 +22,7 @@
 
 #include <cstdlib>
 #include <ctime>
+#include <random>
 
 #include <QClipboard>
 #include <QDir>
@@ -173,7 +174,7 @@ void MainWindow::filter(const QString& filter)
             if (allErrors[i].indexOf("test") > 0)
                 allErrors.removeAt(i);
         }
-        std::random_shuffle(allErrors.begin(), allErrors.end());
+        std::shuffle(allErrors.begin(), allErrors.end(), std::mt19937(std::random_device()()));
         ui->results->addItems(allErrors.mid(0, MAX_ERRORS));
         ui->results->sortItems();
     } else {