Fix #12546 new check: suggest to return const reference from "getter" member function (cppcheck-opensource#6256)

chrchr-github · web-flow · commit 4f8f7ccf7ba3 · 2024-04-10T17:51:50.000+02:00
diff --git a/Makefile b/Makefile
@@ -498,7 +498,7 @@ $(libcppdir)/checkboost.o: lib/checkboost.cpp lib/check.h lib/checkboost.h lib/c
 $(libcppdir)/checkbufferoverrun.o: lib/checkbufferoverrun.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/checkbufferoverrun.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h lib/xml.h
 	$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbufferoverrun.cpp
 
-$(libcppdir)/checkclass.o: lib/checkclass.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/checkclass.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h lib/xml.h
+$(libcppdir)/checkclass.o: lib/checkclass.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/checkclass.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h lib/xml.h
 	$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkclass.cpp
 
 $(libcppdir)/checkcondition.o: lib/checkcondition.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkcondition.h lib/checkother.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
diff --git a/gui/application.h b/gui/application.h
@@ -49,23 +49,23 @@ class Application {
      * @brief Get application name.
      * @return Application name.
      */
-    QString getName() const {
+    const QString& getName() const {
         return mName;
     }
 
     /**
      * @brief Get application path.
      * @return Application path.
      */
-    QString getPath() const {
+    const QString& getPath() const {
         return mPath;
     }
 
     /**
      * @brief Get application command line parameters.
      * @return Application command line parameters.
      */
-    QString getParameters() const {
+    const QString& getParameters() const {
         return mParameters;
     }
 
diff --git a/gui/checkstatistics.h b/gui/checkstatistics.h
@@ -68,7 +68,7 @@ class CheckStatistics : public QObject {
      */
     unsigned getCount(const QString &tool, ShowTypes::ShowType type) const;
 
-    std::set<std::string> getActiveCheckers() const {
+    const std::set<std::string>& getActiveCheckers() const {
         return mActiveCheckers;
     }
 
@@ -82,7 +82,7 @@ class CheckStatistics : public QObject {
     void setCheckersReport(QString report) {
         mCheckersReport = std::move(report);
     }
-    QString getCheckersReport() const {
+    const QString& getCheckersReport() const {
         return mCheckersReport;
     }
 
diff --git a/gui/codeeditor.h b/gui/codeeditor.h
@@ -114,7 +114,7 @@ class CodeEditor : public QPlainTextEdit {
         mFileName = fileName;
     }
 
-    QString getFileName() const {
+    const QString& getFileName() const {
         return mFileName;
     }
 
diff --git a/gui/mainwindow.cpp b/gui/mainwindow.cpp
@@ -1751,7 +1751,7 @@ void MainWindow::analyzeProject(const ProjectFile *projectFile, const bool check
     Settings::terminate(false);
 
     QFileInfo inf(projectFile->getFilename());
-    const QString rootpath = projectFile->getRootPath();
+    const QString& rootpath = projectFile->getRootPath();
 
     QDir::setCurrent(inf.absolutePath());
 
diff --git a/gui/printablereport.cpp b/gui/printablereport.cpp
@@ -52,7 +52,7 @@ void PrintableReport::writeError(const ErrorItem &error)
     mFormattedReport += "\n";
 }
 
-QString PrintableReport::getFormattedReportText() const
+const QString& PrintableReport::getFormattedReportText() const
 {
     return mFormattedReport;
 }
diff --git a/gui/printablereport.h b/gui/printablereport.h
@@ -62,7 +62,7 @@ class PrintableReport : public Report {
     /**
      * @brief Returns the formatted report.
      */
-    QString getFormattedReportText() const;
+    const QString& getFormattedReportText() const;
 
 private:
 
diff --git a/gui/projectfile.h b/gui/projectfile.h
@@ -75,15 +75,15 @@ class ProjectFile : public QObject {
      * @brief Get project root path.
      * @return project root path.
      */
-    QString getRootPath() const {
+    const QString& getRootPath() const {
         return mRootPath;
     }
 
-    QString getBuildDir() const {
+    const QString& getBuildDir() const {
         return mBuildDir;
     }
 
-    QString getImportProject() const {
+    const QString& getImportProject() const {
         return mImportProject;
     }
 
@@ -119,15 +119,15 @@ class ProjectFile : public QObject {
      * @brief Get list of defines.
      * @return list of defines.
      */
-    QStringList getDefines() const {
+    const QStringList& getDefines() const {
         return mDefines;
     }
 
     /**
      * @brief Get list of undefines.
      * @return list of undefines.
      */
-    QStringList getUndefines() const {
+    const QStringList& getUndefines() const {
         return mUndefines;
     }
 
@@ -151,27 +151,27 @@ class ProjectFile : public QObject {
      * @brief Get list of paths to exclude from the check.
      * @return list of paths.
      */
-    QStringList getVsConfigurations() const {
+    const QStringList& getVsConfigurations() const {
         return mVsConfigurations;
     }
 
     /**
      * @brief Get list libraries.
      * @return list of libraries.
      */
-    QStringList getLibraries() const {
+    const QStringList& getLibraries() const {
         return mLibraries;
     }
 
     /**
      * @brief Get platform.
      * @return Current platform. If it ends with .xml then it is a file. Otherwise it must match one of the return values from @sa cppcheck::Platform::toString() ("win32A", "unix32", ..)
      */
-    QString getPlatform() const {
+    const QString& getPlatform() const {
         return mPlatform;
     }
 
-    QString getProjectName() const {
+    const QString& getProjectName() const {
         return mProjectName;
     }
 
@@ -183,7 +183,7 @@ class ProjectFile : public QObject {
      * @brief Get "raw" suppressions.
      * @return list of suppressions.
      */
-    QList<SuppressionList::Suppression> getSuppressions() const {
+    const QList<SuppressionList::Suppression>& getSuppressions() const {
         return mSuppressions;
     }
 
@@ -197,7 +197,7 @@ class ProjectFile : public QObject {
      * @brief Get list addons.
      * @return list of addons.
      */
-    QStringList getAddons() const {
+    const QStringList& getAddons() const {
         return mAddons;
     }
 
@@ -230,7 +230,7 @@ class ProjectFile : public QObject {
         mClangTidy = c;
     }
 
-    QStringList getTags() const {
+    const QStringList& getTags() const {
         return mTags;
     }
 
@@ -254,7 +254,7 @@ class ProjectFile : public QObject {
      * @brief Get filename for the project file.
      * @return file name.
      */
-    QString getFilename() const {
+    const QString& getFilename() const {
         return mFilename;
     }
 
@@ -367,7 +367,7 @@ class ProjectFile : public QObject {
     }
 
     /** @brief Get list of coding standards (checked by Cppcheck Premium). */
-    QStringList getCodingStandards() const {
+    const QStringList& getCodingStandards() const {
         return mCodingStandards;
     }
 
@@ -414,7 +414,7 @@ class ProjectFile : public QObject {
     SafeChecks safeChecks;
 
     /** Check unknown function return values */
-    QStringList getCheckUnknownFunctionReturn() const {
+    const QStringList& getCheckUnknownFunctionReturn() const {
         return mCheckUnknownFunctionReturn;
     }
     /*
diff --git a/gui/projectfiledialog.h b/gui/projectfiledialog.h
@@ -109,7 +109,7 @@ class ProjectFileDialog : public QDialog {
      * @brief Return suppressions from the dialog control.
      * @return List of suppressions.
      */
-    QList<SuppressionList::Suppression> getSuppressions() const {
+    const QList<SuppressionList::Suppression>& getSuppressions() const {
         return mSuppressions;
     }
 
diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp
@@ -1337,7 +1337,7 @@ void ResultsTree::setCheckDirectory(const QString &dir)
 }
 
 
-QString ResultsTree::getCheckDirectory()
+const QString& ResultsTree::getCheckDirectory()
 {
     return mCheckPath;
 }
diff --git a/gui/resultstree.h b/gui/resultstree.h
@@ -134,7 +134,7 @@ class ResultsTree : public QTreeView {
      * @return Directory containing source files
      */
 
-    QString getCheckDirectory();
+    const QString& getCheckDirectory();
 
     /**
      * @brief Check if there are any visible results in view.
diff --git a/gui/translationhandler.cpp b/gui/translationhandler.cpp
@@ -142,7 +142,7 @@ bool TranslationHandler::setLanguage(const QString &code)
     return true;
 }
 
-QString TranslationHandler::getCurrentLanguage() const
+const QString& TranslationHandler::getCurrentLanguage() const
 {
     return mCurrentLanguage;
 }
diff --git a/gui/translationhandler.h b/gui/translationhandler.h
@@ -69,7 +69,7 @@ class TranslationHandler : QObject {
      * @return List of available translations.
      *
      */
-    QList<TranslationInfo> getTranslations() const {
+    const QList<TranslationInfo>& getTranslations() const {
         return mTranslations;
     }
 
@@ -86,7 +86,7 @@ class TranslationHandler : QObject {
      * @return ISO 639 language code for current translation.
      *
      */
-    QString getCurrentLanguage() const;
+    const QString& getCurrentLanguage() const;
 
     /**
      * @brief Get translation suggestion for the system.
diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp
@@ -30,6 +30,7 @@
 #include "tokenize.h"
 #include "tokenlist.h"
 #include "utils.h"
+#include "valueflow.h"
 
 #include <algorithm>
 #include <cctype>
@@ -3303,6 +3304,54 @@ void CheckClass::checkUselessOverride()
     }
 }
 
+static const Variable* getSingleReturnVar(const Scope* scope) {
+    if (!scope)
+        return nullptr;
+    const Token* const start = scope->bodyStart->next();
+    const Token* const end = Token::findsimplematch(start, ";", 1, scope->bodyEnd);
+    if (!end || end->next() != scope->bodyEnd)
+        return nullptr;
+    if (!start->astOperand1() || start->str() != "return")
+        return nullptr;
+    return start->astOperand1()->variable();
+}
+
+void CheckClass::checkReturnByReference()
+{
+    if (!mSettings->severity.isEnabled(Severity::performance) && !mSettings->isPremiumEnabled("returnByReference"))
+        return;
+
+    logChecker("CheckClass::checkReturnByReference"); // performance
+
+    for (const Scope* classScope : mSymbolDatabase->classAndStructScopes) {
+        for (const Function& func : classScope->functionList) {
+            if (Function::returnsPointer(&func) || Function::returnsReference(&func) || Function::returnsStandardType(&func))
+                continue;
+            if (const Variable* var = getSingleReturnVar(func.functionScope)) {
+                if (!var->valueType())
+                    continue;
+                const bool isContainer = var->valueType()->type == ValueType::Type::CONTAINER && var->valueType()->container;
+                const bool isView = isContainer && var->valueType()->container->view;
+                bool warn = isContainer && !isView;
+                if (!warn && !isView) {
+                    const std::size_t size = ValueFlow::getSizeOf(*var->valueType(), *mSettings);
+                    if (size > 2 * mSettings->platform.sizeof_pointer)
+                        warn = true;
+                }
+                if (warn)
+                    returnByReferenceError(&func, var);
+            }
+        }
+    }
+}
+
+void CheckClass::returnByReferenceError(const Function* func, const Variable* var)
+{
+    const Token* tok = func ? func->tokenDef : nullptr;
+    const std::string message = "Function '" + (func ? func->name() : "func") + "()' should return member '" + (var ? var->name() : "var") + "' by const reference.";
+    reportError(tok, Severity::performance, "returnByReference", message);
+}
+
 void CheckClass::checkThisUseAfterFree()
 {
     if (!mSettings->severity.isEnabled(Severity::warning))
diff --git a/lib/checkclass.h b/lib/checkclass.h
@@ -91,6 +91,7 @@ class CPPCHECKLIB CheckClass : public Check {
         checkClass.checkCopyCtorAndEqOperator();
         checkClass.checkOverride();
         checkClass.checkUselessOverride();
+        checkClass.checkReturnByReference();
         checkClass.checkThisUseAfterFree();
         checkClass.checkUnsafeClassRefMember();
     }
@@ -157,6 +158,9 @@ class CPPCHECKLIB CheckClass : public Check {
     /** @brief Check that the overriden function is not identical to the base function */
     void checkUselessOverride();
 
+    /** @brief Check that large members are returned by reference from getter function */
+    void checkReturnByReference();
+
     /** @brief When "self pointer" is destroyed, 'this' might become invalid. */
     void checkThisUseAfterFree();
 
@@ -208,6 +212,7 @@ class CPPCHECKLIB CheckClass : public Check {
     void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor);
     void overrideError(const Function *funcInBase, const Function *funcInDerived);
     void uselessOverrideError(const Function *funcInBase, const Function *funcInDerived, bool isSameCode = false);
+    void returnByReferenceError(const Function *func, const Variable* var);
     void thisUseAfterFree(const Token *self, const Token *free, const Token *use);
     void unsafeClassRefMemberError(const Token *tok, const std::string &varname);
     void checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase);
@@ -246,9 +251,11 @@ class CPPCHECKLIB CheckClass : public Check {
         c.selfInitializationError(nullptr, "var");
         c.duplInheritedMembersError(nullptr, nullptr, "class", "class", "variable", false, false);
         c.copyCtorAndEqOperatorError(nullptr, "class", false, false);
+        c.overrideError(nullptr, nullptr);
+        c.uselessOverrideError(nullptr, nullptr);
+        c.returnByReferenceError(nullptr, nullptr);
         c.pureVirtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
         c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
-        c.overrideError(nullptr, nullptr);
         c.thisUseAfterFree(nullptr, nullptr, nullptr);
         c.unsafeClassRefMemberError(nullptr, "UnsafeClass::var");
     }
diff --git a/lib/platform.h b/lib/platform.h
@@ -88,17 +88,17 @@ class CPPCHECKLIB Platform {
     nonneg int long_long_bit;  /// bits in long long
 
     /** size of standard types */
-    nonneg int sizeof_bool;
-    nonneg int sizeof_short;
-    nonneg int sizeof_int;
-    nonneg int sizeof_long;
-    nonneg int sizeof_long_long;
-    nonneg int sizeof_float;
-    nonneg int sizeof_double;
-    nonneg int sizeof_long_double;
-    nonneg int sizeof_wchar_t;
-    nonneg int sizeof_size_t;
-    nonneg int sizeof_pointer;
+    std::size_t sizeof_bool;
+    std::size_t sizeof_short;
+    std::size_t sizeof_int;
+    std::size_t sizeof_long;
+    std::size_t sizeof_long_long;
+    std::size_t sizeof_float;
+    std::size_t sizeof_double;
+    std::size_t sizeof_long_double;
+    std::size_t sizeof_wchar_t;
+    std::size_t sizeof_size_t;
+    std::size_t sizeof_pointer;
 
     char defaultSign;  // unsigned:'u', signed:'s', unknown:'\0'
 
diff --git a/lib/templatesimplifier.h b/lib/templatesimplifier.h
diff --git a/oss-fuzz/Makefile b/oss-fuzz/Makefile
diff --git a/releasenotes.txt b/releasenotes.txt
diff --git a/test/testclass.cpp b/test/testclass.cpp

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ class CheckStatistics : public QObject {`
`68`	`68`	`*/`
`69`	`69`	`unsigned getCount(const QString &tool, ShowTypes::ShowType type) const;`
`70`	`70`
`71`		`- std::set<std::string> getActiveCheckers() const {`
	`71`	`+ const std::set<std::string>& getActiveCheckers() const {`
`72`	`72`	`return mActiveCheckers;`
`73`	`73`	`}`
`74`	`74`
`@@ -82,7 +82,7 @@ class CheckStatistics : public QObject {`
`82`	`82`	`void setCheckersReport(QString report) {`
`83`	`83`	`mCheckersReport = std::move(report);`
`84`	`84`	`}`
`85`		`- QString getCheckersReport() const {`
	`85`	`+ const QString& getCheckersReport() const {`
`86`	`86`	`return mCheckersReport;`
`87`	`87`	`}`
`88`	`88`
Original file line number	Diff line number	Diff line change
`@@ -114,7 +114,7 @@ class CodeEditor : public QPlainTextEdit {`
`114`	`114`	`mFileName = fileName;`
`115`	`115`	`}`
`116`	`116`
`117`		`- QString getFileName() const {`
	`117`	`+ const QString& getFileName() const {`
`118`	`118`	`return mFileName;`
`119`	`119`	`}`
`120`	`120`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ void PrintableReport::writeError(const ErrorItem &error)`
`52`	`52`	`mFormattedReport += "\n";`
`53`	`53`	`}`
`54`	`54`
`55`		`-QString PrintableReport::getFormattedReportText() const`
	`55`	`+const QString& PrintableReport::getFormattedReportText() const`
`56`	`56`	`{`
`57`	`57`	`return mFormattedReport;`
`58`	`58`	`}`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ class ProjectFileDialog : public QDialog {`
`109`	`109`	`* @brief Return suppressions from the dialog control.`
`110`	`110`	`* @return List of suppressions.`
`111`	`111`	`*/`
`112`		`- QList<SuppressionList::Suppression> getSuppressions() const {`
	`112`	`+ const QList<SuppressionList::Suppression>& getSuppressions() const {`
`113`	`113`	`return mSuppressions;`
`114`	`114`	`}`
`115`	`115`
Original file line number	Diff line number	Diff line change
`@@ -1337,7 +1337,7 @@ void ResultsTree::setCheckDirectory(const QString &dir)`
`1337`	`1337`	`}`
`1338`	`1338`
`1339`	`1339`
`1340`		`-QString ResultsTree::getCheckDirectory()`
	`1340`	`+const QString& ResultsTree::getCheckDirectory()`
`1341`	`1341`	`{`
`1342`	`1342`	`return mCheckPath;`
`1343`	`1343`	`}`