diff --git a/Makefile b/Makefile index b59a0772624..b4396864969 100644 --- 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 index e6d92ee853b..d624ecf23e1 100644 --- a/gui/application.h +++ b/gui/application.h @@ -49,7 +49,7 @@ class Application { * @brief Get application name. * @return Application name. */ - QString getName() const { + const QString& getName() const { return mName; } @@ -57,7 +57,7 @@ class Application { * @brief Get application path. * @return Application path. */ - QString getPath() const { + const QString& getPath() const { return mPath; } @@ -65,7 +65,7 @@ class Application { * @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 index 65fa7783b2f..9e064184d99 100644 --- 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 getActiveCheckers() const { + const std::set& 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 index 6e06db9881f..e301f6bf8d7 100644 --- 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 index 917f17018de..9a8ba14504a 100644 --- 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 index d9272632f45..b759c513628 100644 --- 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 index 693356a961d..a5a4a975e5b 100644 --- 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 index 6c70e6f12bd..328015bb5e2 100644 --- 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,7 +119,7 @@ class ProjectFile : public QObject { * @brief Get list of defines. * @return list of defines. */ - QStringList getDefines() const { + const QStringList& getDefines() const { return mDefines; } @@ -127,7 +127,7 @@ class ProjectFile : public QObject { * @brief Get list of undefines. * @return list of undefines. */ - QStringList getUndefines() const { + const QStringList& getUndefines() const { return mUndefines; } @@ -151,7 +151,7 @@ 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; } @@ -159,7 +159,7 @@ class ProjectFile : public QObject { * @brief Get list libraries. * @return list of libraries. */ - QStringList getLibraries() const { + const QStringList& getLibraries() const { return mLibraries; } @@ -167,11 +167,11 @@ class ProjectFile : public QObject { * @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 getSuppressions() const { + const QList& 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 index fe243e15137..d9edef245f5 100644 --- 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 getSuppressions() const { + const QList& getSuppressions() const { return mSuppressions; } diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp index a14a7a15351..eb61a08cb87 100644 --- 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 index 8d855766082..213bb7fcdaa 100644 --- 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 index 0caa3dcb1c5..173da1aca94 100644 --- 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 index cd298d6ebaf..12ed2d4a6c0 100644 --- a/gui/translationhandler.h +++ b/gui/translationhandler.h @@ -69,7 +69,7 @@ class TranslationHandler : QObject { * @return List of available translations. * */ - QList getTranslations() const { + const QList& 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 index 627f15574b7..1a42f8ab820 100644 --- 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 #include @@ -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 index 5a84e4cbdaf..bc2a9d5116d 100644 --- 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(), "f"); c.virtualFunctionCallInConstructorError(nullptr, std::list(), "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 index 5e4e51e71f6..6c2d17dfb9d 100644 --- 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 index e8e77507e5e..d3a7bcd6cf6 100644 --- a/lib/templatesimplifier.h +++ b/lib/templatesimplifier.h @@ -49,7 +49,7 @@ class CPPCHECKLIB TemplateSimplifier { public: explicit TemplateSimplifier(Tokenizer &tokenizer); - std::string dump() const { + const std::string& dump() const { return mDump; } diff --git a/oss-fuzz/Makefile b/oss-fuzz/Makefile index 72ce6c43581..88c76f4e134 100644 --- a/oss-fuzz/Makefile +++ b/oss-fuzz/Makefile @@ -178,7 +178,7 @@ $(libcppdir)/checkboost.o: ../lib/checkboost.cpp ../lib/check.h ../lib/checkboos $(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) ${LIB_FUZZING_ENGINE} $(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) ${LIB_FUZZING_ENGINE} $(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/releasenotes.txt b/releasenotes.txt index 56c7500bc41..bf159e7a5e3 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -2,6 +2,7 @@ Release Notes for Cppcheck 2.14 New checks: eraseIteratorOutOfBounds: warns when erase() is called on an iterator that is out of bounds +returnByReference: warns when a large class member is returned by value from a getter function Improved checking: - diff --git a/test/testclass.cpp b/test/testclass.cpp index 989dfa516a0..4d62db1eadb 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -242,6 +242,8 @@ class TestClass : public TestFixture { TEST_CASE(ctuOneDefinitionRule); TEST_CASE(testGetFileInfo); + + TEST_CASE(returnByReference); } #define checkCopyCtorAndEqOperator(code) checkCopyCtorAndEqOperator_(code, __FILE__, __LINE__) @@ -8942,6 +8944,37 @@ class TestClass : public TestFixture { getFileInfo("struct sometype { sometype(); }; sometype::sometype() = delete;"); // don't crash } +#define checkReturnByReference(...) checkReturnByReference_(__FILE__, __LINE__, __VA_ARGS__) + void checkReturnByReference_(const char* file, int line, const char code[]) { + const Settings settings = settingsBuilder().severity(Severity::performance).library("std.cfg").build(); + + std::vector files(1, "test.cpp"); + Tokenizer tokenizer(settings, this); + PreprocessorHelper::preprocess(code, files, tokenizer); + + ASSERT_LOC(tokenizer.simplifyTokens1(""), file, line); + + // Check.. + CheckClass checkClass(&tokenizer, &settings, this); + (checkClass.checkReturnByReference)(); + } + + void returnByReference() { + checkReturnByReference("struct T { int a[10]; };" // #12546 + "struct S {" + " T t;" + " int i;" + " std::string s;" + " T getT() const { return t; }" + " int getI() const { return i; }" + " std::string getS() const { return s; }" + " unknown_t f() { return; }" + "};"); + ASSERT_EQUALS("[test.cpp:1]: (performance) Function 'getT()' should return member 't' by const reference.\n" + "[test.cpp:1]: (performance) Function 'getS()' should return member 's' by const reference.\n", + errout_str()); + } + }; REGISTER_TEST(TestClass)