From 2ca932a3ae3c9c051c23dacbcc3ed3e622aeddca Mon Sep 17 00:00:00 2001
From: Richard Quirk <richard.quirk@gmail.com>
Date: Fri, 28 Oct 2011 22:08:12 +0200
Subject: [PATCH] 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> 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)