From b88d61dcb4afb3ef29dc6e39ad68d57783886b79 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Fri, 28 Oct 2011 22:06:57 +0200 Subject: [PATCH 1/3] Make single-argument test constructors explicit --- test/testclass.cpp | 18 +++++++++--------- test/testconstructors.cpp | 24 ++++++++++++------------ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index 6b53fc10d..d2d7c8817 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -1683,7 +1683,7 @@ private: " FILE *fp;\n" "\n" "public:\n" - " C(FILE *fp);\n" + " explicit C(FILE *fp);\n" "};\n" "\n" "C::C(FILE *fp) {\n" @@ -1761,7 +1761,7 @@ private: checkUninitVar("class Foo : public Bar\n" "{\n" "public:\n" - " Foo(int i) : Bar(mi=i) { }\n" + " explicit Foo(int i) : Bar(mi=i) { }\n" "private:\n" " int mi;\n" "};\n"); @@ -1811,7 +1811,7 @@ private: "private:\n" " int xasd;\n" "public:\n" - " Prefs(wxSize size);\n" + " explicit Prefs(wxSize size);\n" "};\n" "Prefs::Prefs(wxSize size)\n" "{\n" @@ -1835,7 +1835,7 @@ private: void uninitVar11() { checkUninitVar("class A {\n" "public:\n" - " A(int a = 0);\n" + " explicit A(int a = 0);\n" "private:\n" " int var;\n" "};\n" @@ -2138,7 +2138,7 @@ private: void uninitVar18() { // ticket #2465 checkUninitVar("struct Altren\n" "{\n" - " Altren(int _a = 0) : value(0) { }\n" + " explicit Altren(int _a = 0) : value(0) { }\n" " int value;\n" "};\n" "class A\n" @@ -2152,7 +2152,7 @@ private: checkUninitVar("struct Altren\n" "{\n" - " Altren(int _a) : value(0) { }\n" + " explicit Altren(int _a) : value(0) { }\n" " int value;\n" "};\n" "class A\n" @@ -2171,7 +2171,7 @@ private: " char* m_str;\n" " int m_len;\n" "public:\n" - " mystring(const char* str)\n" + " explicit mystring(const char* str)\n" " {\n" " m_len = strlen(str);\n" " m_str = (char*) malloc(m_len+1);\n" @@ -2602,7 +2602,7 @@ private: "private:\n" " int foo;\n" "public:\n" - " Foo(std::istream &in)\n" + " explicit Foo(std::istream &in)\n" " {\n" " if(!(in >> foo))\n" " throw 0;\n" @@ -2659,7 +2659,7 @@ private: " int foo;\n" " Foo() { }\n" "public:\n" - " Foo(int _i) { }\n" + " explicit Foo(int _i) { }\n" "};\n"); ASSERT_EQUALS("[test.cpp:7]: (warning) Member variable 'Foo::foo' is not initialized in the constructor.\n", errout.str()); diff --git a/test/testconstructors.cpp b/test/testconstructors.cpp index 1c20bcd36..49d485349 100644 --- a/test/testconstructors.cpp +++ b/test/testconstructors.cpp @@ -227,7 +227,7 @@ private: "{\n" "public:\n" " Fred();\n" - " Fred(int _i);\n" + " explicit Fred(int _i);\n" " int i;\n" "};\n" "Fred::Fred()\n" @@ -241,7 +241,7 @@ private: check("struct Fred\n" "{\n" " Fred();\n" - " Fred(int _i);\n" + " explicit Fred(int _i);\n" " int i;\n" "};\n" "Fred::Fred()\n" @@ -647,7 +647,7 @@ private: check("class c\n" "{\n" " c();\n" - " c(bool b);" + " explicit c(bool b);" "\n" " void InitInt();\n" "\n" @@ -677,7 +677,7 @@ private: check("struct c\n" "{\n" " c();\n" - " c(bool b);" + " explicit c(bool b);" "\n" " void InitInt();\n" "\n" @@ -850,12 +850,12 @@ private: "public:\n" " A();\n" " struct B {\n" - " B(int x);\n" + " explicit B(int x);\n" " struct C {\n" - " C(int y);\n" + " explicit C(int y);\n" " struct D {\n" " int d;\n" - " D(int z);\n" + " explicit D(int z);\n" " };\n" " int c;\n" " };\n" @@ -879,9 +879,9 @@ private: "public:\n" " A();\n" " struct B {\n" - " B(int x);\n" + " explicit B(int x);\n" " struct C {\n" - " C(int y);\n" + " explicit C(int y);\n" " struct D {\n" " D(const D &);\n" " int d;\n" @@ -908,13 +908,13 @@ private: "public:\n" " A();\n" " struct B {\n" - " B(int x);\n" + " explicit B(int x);\n" " struct C {\n" - " C(int y);\n" + " explicit C(int y);\n" " struct D {\n" " struct E { int e; };\n" " struct E d;\n" - " D(const E &);\n" + " explicit D(const E &);\n" " };\n" " int c;\n" " };\n" From 2ca932a3ae3c9c051c23dacbcc3ed3e622aeddca Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Fri, 28 Oct 2011 22:08:12 +0200 Subject: [PATCH 2/3] Add checks for explicit constructors Single-argument constructors should be explicit. Constructors with multiple arguments should not be marked explicit. --- lib/checkclass.cpp | 34 ++++++++++++++++++++++++++++++++++ lib/checkclass.h | 4 ++++ test/testconstructors.cpp | 17 +++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a5b571fd7..7740e856b 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -90,6 +90,14 @@ void CheckClass::constructors() std::vector usage(scope->varlist.size()); for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + // check for explicit + if (func->type == Function::eConstructor) { + if (!func->isExplicit && func->argumentList.size() == 1) + explicitConstructorError(func->token, scope->className); + if (func->isExplicit && func->argumentList.size() > 1) + pointlessExplicitConstructorError(func->token, scope->className); + } + if (!func->hasBody || !(func->type == Function::eConstructor || func->type == Function::eCopyConstructor || func->type == Function::eOperatorEqual)) @@ -515,6 +523,32 @@ void CheckClass::operatorEqVarError(const Token *tok, const std::string &classna reportError(tok, Severity::warning, "operatorEqVarError", "Member variable '" + classname + "::" + varname + "' is not assigned a value in '" + classname + "::operator=" + "'"); } +void CheckClass::explicitConstructorError(const Token *tok, const std::string &className) +{ + reportError(tok, Severity::style, + "explicitConstructorError", "Constructor for '" + + className + "' should be explicit.\n" + "The single-argument constructor for '" + className + "' should " + "be explicit as it can be used for automatic conversion. This is " + "convenient but can also be a problem when automatic conversion " + "creates new objects when you were not expecting it. Adding the " + "explicit declaration to the constructor prevents it being called " + "for implicit conversions."); +} + +void CheckClass::pointlessExplicitConstructorError(const Token *tok, const std::string &className) +{ + reportError(tok, Severity::style, + "pointlessExplicitConstructorError", "Constructor for '" + + className + "' is marked explicit but" + " takes more than one argument.\n" + "The explicit keyword prevents constructor calls for implicit " + "conversions, but it is only needed for single-argument " + "constructors. The constructor for '" + className + "' takes " + "more than one argument so is not affected by the explicit " + "declaration."); +} + //--------------------------------------------------------------------------- // ClassCheck: Unused private functions //--------------------------------------------------------------------------- diff --git a/lib/checkclass.h b/lib/checkclass.h index a5d4b7ae1..9e8fb1ea8 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -128,6 +128,8 @@ private: void checkConstError(const Token *tok, const std::string &classname, const std::string &funcname); void checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname); void initializerListError(const Token *tok1,const Token *tok2, const std::string & classname, const std::string &varname); + void explicitConstructorError(const Token *tok, const std::string &className); + void pointlessExplicitConstructorError(const Token *tok, const std::string &className); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { CheckClass c(0, settings, errorLogger); @@ -143,6 +145,8 @@ private: c.operatorEqToSelfError(0); c.checkConstError(0, "class", "function"); c.initializerListError(0, 0, "class", "variable"); + c.explicitConstructorError(0, "classname"); + c.pointlessExplicitConstructorError(0, "classname"); } std::string myName() const { diff --git a/test/testconstructors.cpp b/test/testconstructors.cpp index 49d485349..927b47c34 100644 --- a/test/testconstructors.cpp +++ b/test/testconstructors.cpp @@ -81,6 +81,7 @@ private: TEST_CASE(initvar_destructor); // No variables need to be initialized in a destructor TEST_CASE(operatorEqSTL); + TEST_CASE(explicit_constructor); } @@ -1043,6 +1044,22 @@ private: "{ }", true); ASSERT_EQUALS("[test.cpp:13]: (warning) Member variable 'Fred::ints' is not assigned a value in 'Fred::operator='\n", errout.str()); } + + void explicit_constructor() + { + check("class Fred\n" + "{\n" + " Fred(int i);\n" + "};\n" + "\n", true); + ASSERT_EQUALS("[test.cpp:3]: (style) Constructor for 'Fred' should be explicit.\n", errout.str()); + check("class Fred\n" + "{\n" + " explicit Fred(int a, int b);\n" + "};\n" + "\n", true); + ASSERT_EQUALS("[test.cpp:3]: (style) Constructor for 'Fred' is marked explicit but takes more than one argument.\n", errout.str()); + } }; REGISTER_TEST(TestConstructors) From a0a5b36667c3b6b439619eeba5b090ca8943f387 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Fri, 28 Oct 2011 22:10:19 +0200 Subject: [PATCH 3/3] Ensure single-argument constructors are explicit --- lib/check.h | 2 +- lib/checkbufferoverrun.cpp | 2 +- lib/checknullpointer.cpp | 2 +- lib/checkuninitvar.cpp | 2 +- lib/preprocessor.cpp | 2 +- lib/token.h | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/check.h b/lib/check.h index 2cf53b402..b438c79bf 100644 --- a/lib/check.h +++ b/lib/check.h @@ -38,7 +38,7 @@ class Check { public: /** This constructor is used when registering the CheckClass */ - Check(const std::string &aname); + explicit Check(const std::string &aname); /** This constructor is used when running checks. */ Check(const std::string &aname, const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 3985765d9..92081d767 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -191,7 +191,7 @@ public: /** * @param str Token::str() is compared against this. */ - TokenStrEquals(const std::string &str) + explicit TokenStrEquals(const std::string &str) : value(str) { } diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 8cc92bb28..15a0ee812 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -940,7 +940,7 @@ void CheckNullPointer::nullConstantDereference() class Nullpointer : public ExecutionPath { public: /** Startup constructor */ - Nullpointer(Check *c) : ExecutionPath(c, 0), null(false) { + explicit Nullpointer(Check *c) : ExecutionPath(c, 0), null(false) { } private: diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index a03da46cc..c43afbd71 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -41,7 +41,7 @@ namespace { class UninitVar : public ExecutionPath { public: /** Startup constructor */ - UninitVar(Check *c) + explicit UninitVar(Check *c) : ExecutionPath(c, 0), pointer(false), array(false), alloc(false), strncpy_(false), memset_nonzero(false) { } diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index 4035876bb..5b3432044 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -2167,7 +2167,7 @@ public: * @param macro The code after define, until end of line, * e.g. "A(x) foo(x);" */ - PreprocessorMacro(const std::string ¯o) + explicit PreprocessorMacro(const std::string ¯o) : _macro(macro), _prefix("__cppcheck__") { tokenizer.setSettings(&settings); diff --git a/lib/token.h b/lib/token.h index 3d61fd07f..a1425ea5a 100644 --- a/lib/token.h +++ b/lib/token.h @@ -43,7 +43,7 @@ private: Token(); public: - Token(Token **tokensBack); + explicit Token(Token **tokensBack); ~Token(); void str(const std::string &s);