From 2af9212b16a37d96aa713348ecd37aa46bfdcbe2 Mon Sep 17 00:00:00 2001 From: Jakub Melka Date: Sat, 7 Mar 2015 20:07:54 +0100 Subject: [PATCH] Ticket #695: new style check : explicit declaration of ctor --- lib/checkclass.cpp | 56 +++++++++++++++++++++++++++++++++++++ lib/checkclass.h | 10 +++++++ test/testclass.cpp | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index b2bce9653..b9dc6a9d7 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -217,6 +217,52 @@ void CheckClass::constructors() } } +void CheckClass::checkExplicitConstructors() +{ + if (!_settings->isEnabled("performance")) + return; + + const std::size_t classes = symbolDatabase->classAndStructScopes.size(); + for (std::size_t i = 0; i < classes; ++i) { + const Scope * scope = symbolDatabase->classAndStructScopes[i]; + + // Do not perform check, if the class/struct has not any constructors + if (scope->numConstructors == 0) + continue; + + // Is class abstract? Maybe this test is over-simplification, but it will suffice for simple cases, + // and it will avoid false positives. + bool isAbstractClass = false; + for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + if (func->isPure()) { + isAbstractClass = true; + break; + } + } + + for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + + // We are looking for constructors, which are meeting following criteria: + // 1) Constructor is declared with a single parameter + // 2) Constructor is not declared as explicit + // 3) It is not a copy/move constructor of non-abstract class + // 4) Constructor is not marked as delete (programmer can mark the default constructor as deleted, which is ok) + if (!func->isConstructor() || func->isDelete()) + continue; + + if (!func->isExplicit() && func->argCount() == 1) { + // We must decide, if it is not a copy/move constructor, or it is a copy/move constructor of abstract class. + if (func->type != Function::eCopyConstructor && func->type != Function::eMoveConstructor) { + noExplicitConstructorError(func->tokenDef, scope->className, scope->type == Scope::eStruct); + } + else if (isAbstractClass) { + noExplicitCopyMoveConstructorError(func->tokenDef, scope->className, scope->type == Scope::eStruct); + } + } + } + } +} + void CheckClass::copyconstructors() { if (!_settings->isEnabled("style")) @@ -723,6 +769,16 @@ void CheckClass::noConstructorError(const Token *tok, const std::string &classna "instantiated. That may cause bugs or undefined behavior."); } +void CheckClass::noExplicitConstructorError(const Token *tok, const std::string &classname, bool isStruct) +{ + reportError(tok, Severity::performance, "noExplicitConstructor", "The constructor of " + std::string(isStruct ? "struct" : "class") + " '" + classname + "' should be marked as explicit. "); +} + +void CheckClass::noExplicitCopyMoveConstructorError(const Token *tok, const std::string &classname, bool isStruct) +{ + reportError(tok, Severity::performance, "noExplicitCopyMoveConstructor", "The copy/move constructor of abstract " + std::string(isStruct ? "struct" : "class") + " '" + classname + "' should be marked as explicit. "); +} + void CheckClass::uninitVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive) { reportError(tok, Severity::warning, "uninitMemberVar", "Member variable '" + classname + "::" + varname + "' is not initialized in the constructor.", inconclusive); diff --git a/lib/checkclass.h b/lib/checkclass.h index 7b0ad9d67..490f98431 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -76,12 +76,17 @@ public: checkClass.checkPureVirtualFunctionCall(); checkClass.checkDuplInheritedMembers(); + checkClass.checkExplicitConstructors(); } /** @brief %Check that all class constructors are ok */ void constructors(); + /** @brief %Check that constructors with single parameter are explicit, + * if they has to be.*/ + void checkExplicitConstructors(); + /** @brief %Check that all private functions are called */ void privateFunctions(); @@ -136,6 +141,8 @@ private: // Reporting errors.. void noConstructorError(const Token *tok, const std::string &classname, bool isStruct); + void noExplicitConstructorError(const Token *tok, const std::string &classname, bool isStruct); + void noExplicitCopyMoveConstructorError(const Token *tok, const std::string &classname, bool isStruct); //void copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& var_name); void copyConstructorShallowCopyError(const Token *tok, const std::string& varname); void noCopyConstructorError(const Token *tok, const std::string &classname, bool isStruct); @@ -165,6 +172,8 @@ private: void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckClass c(0, settings, errorLogger); c.noConstructorError(0, "classname", false); + c.noExplicitConstructorError(0, "classname", false); + c.noExplicitCopyMoveConstructorError(0, "classname", false); //c.copyConstructorMallocError(0, 0, "var"); c.copyConstructorShallowCopyError(0, "var"); c.noCopyConstructorError(0, "class", false); @@ -199,6 +208,7 @@ private: return "Check the code for each class.\n" "- Missing constructors and copy constructors\n" //"- Missing allocation of memory in copy constructor\n" + "- Constructors which should be explicit are explicit.\n" "- Are all variables initialized by the constructors?\n" "- Are all variables assigned by 'operator='?\n" "- Warn if memset, memcpy etc are used on a class\n" diff --git a/test/testclass.cpp b/test/testclass.cpp index c1ec6f245..f9dd91f82 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -180,6 +180,76 @@ private: TEST_CASE(pureVirtualFunctionCallPrevented); TEST_CASE(duplInheritedMembers); + TEST_CASE(explicitConstructors); + } + + + void checkExplicitConstructors(const char code[]) { + // Clear the error log + errout.str(""); + Settings settings; + settings.addEnabled("performance"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + tokenizer.simplifyTokenList2(); + + // Check.. + CheckClass checkClass(&tokenizer, &settings, this); + checkClass.checkExplicitConstructors(); + } + + void explicitConstructors() { + checkExplicitConstructors("class Class \n" + "{ \n" + " Class() = delete; \n" + " Class(const Class& other) { } \n" + " Class(Class&& other) { } \n" + " explicit Class(int i) { } \n" + " explicit Class(const std::string&) { } \n" + " Class(int a, int b) { } \n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkExplicitConstructors("class Class \n" + "{ \n" + " Class() = delete; \n" + " explicit Class(const Class& other) { } \n" + " explicit Class(Class&& other) { } \n" + " virtual int i() = 0; \n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkExplicitConstructors("class Class \n" + "{ \n" + " Class() = delete; \n" + " Class(const Class& other) = delete; \n" + " Class(Class&& other) = delete; \n" + " virtual int i() = 0; \n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkExplicitConstructors("class Class \n" + "{ \n" + " Class(int i) { } \n" + "};"); + ASSERT_EQUALS("[test.cpp:3]: (performance) The constructor of class 'Class' should be marked as explicit. \n", errout.str()); + + checkExplicitConstructors("class Class \n" + "{ \n" + " Class(const Class& other) { } \n" + " virtual int i() = 0; \n" + "};"); + ASSERT_EQUALS("[test.cpp:3]: (performance) The copy/move constructor of abstract class 'Class' should be marked as explicit. \n", errout.str()); + + checkExplicitConstructors("class Class \n" + "{ \n" + " Class(Class&& other) { } \n" + " virtual int i() = 0; \n" + "};"); + ASSERT_EQUALS("[test.cpp:3]: (performance) The copy/move constructor of abstract class 'Class' should be marked as explicit. \n", errout.str()); } void checkDuplInheritedMembers(const char code[]) {