Merge pull request #51 from richq/explicit

Explicit constructors

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)

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) {
     }
 

lib/checkclass.cpp

@@ -90,6 +90,14 @@ void CheckClass::constructors()
         std::vector<Usage> 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
 //---------------------------------------------------------------------------

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 {

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:

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) {
     }
 

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 &macro)
+    explicit PreprocessorMacro(const std::string &macro)
         : _macro(macro), _prefix("__cppcheck__") {
         tokenizer.setSettings(&settings);
 

lib/token.h

@@ -43,7 +43,7 @@ private:
     Token();
 
 public:
-    Token(Token **tokensBack);
+    explicit Token(Token **tokensBack);
     ~Token();
 
     void str(const std::string &s);

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());

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);
     }
 
 
@@ -227,7 +228,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 +242,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 +648,7 @@ private:
         check("class c\n"
               "{\n"
               "    c();\n"
-              "    c(bool b);"
+              "    explicit c(bool b);"
               "\n"
               "    void InitInt();\n"
               "\n"
@@ -677,7 +678,7 @@ private:
         check("struct c\n"
               "{\n"
               "    c();\n"
-              "    c(bool b);"
+              "    explicit c(bool b);"
               "\n"
               "    void InitInt();\n"
               "\n"
@@ -850,12 +851,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 +880,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 +909,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"
@@ -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)