Merge pull request #540 from JakubMelka/ticket_695

Ticket #695: new style check : explicit declaration of ctor

gui/applicationlist.h

@@ -33,7 +33,7 @@ class ApplicationList : public QObject {
     Q_OBJECT
 public:
 
-    ApplicationList(QObject *parent = 0);
+    explicit ApplicationList(QObject *parent = 0);
     virtual ~ApplicationList();
 
     /**

gui/checkstatistics.h

@@ -30,7 +30,7 @@
  */
 class CheckStatistics : public QObject {
 public:
-    CheckStatistics(QObject *parent = NULL);
+    explicit CheckStatistics(QObject *parent = NULL);
 
     /**
     * @brief Add new checked item to statistics.

gui/checkthread.h

@@ -36,7 +36,7 @@ class Settings;
 class CheckThread : public QThread {
     Q_OBJECT
 public:
-    CheckThread(ThreadResult &result);
+    explicit CheckThread(ThreadResult &result);
     virtual ~CheckThread();
 
     /**

gui/csvreport.h

@@ -35,7 +35,7 @@
 */
 class CsvReport : public Report {
 public:
-    CsvReport(const QString &filename);
+    explicit CsvReport(const QString &filename);
     virtual ~CsvReport();
 
     /**

gui/erroritem.h

@@ -57,7 +57,7 @@ public:
 class ErrorItem {
 public:
     ErrorItem();
-    ErrorItem(const ErrorLine &line);
+    explicit ErrorItem(const ErrorLine &line);
 
     /**
     * @brief Convert error item to string.

gui/logview.h

@@ -32,7 +32,7 @@
 class LogView : public QWidget {
     Q_OBJECT
 public:
-    LogView(QWidget *parent = 0);
+    explicit LogView(QWidget *parent = 0);
     ~LogView();
 
     /**

gui/platforms.h

@@ -44,7 +44,7 @@ class Platforms : public QObject {
     Q_OBJECT
 
 public:
-    Platforms(QObject *parent = NULL);
+    explicit Platforms(QObject *parent = NULL);
     void add(const QString &title, Settings::PlatformType platform);
     int getCount() const;
     void init();

gui/project.h

@@ -36,7 +36,7 @@ class Project : public QObject {
     Q_OBJECT
 
 public:
-    Project(QWidget *parent = 0);
+    explicit Project(QWidget *parent = 0);
     Project(const QString &filename, QWidget *parent = 0);
     ~Project();
 

gui/projectfile.h

@@ -37,7 +37,7 @@ class ProjectFile : public QObject {
     Q_OBJECT
 
 public:
-    ProjectFile(QObject *parent = 0);
+    explicit ProjectFile(QObject *parent = 0);
     ProjectFile(const QString &filename, QObject *parent = 0);
 
     /**

gui/report.h

@@ -40,7 +40,7 @@ public:
         CSV,
     };
 
-    Report(const QString &filename);
+    explicit Report(const QString &filename);
     virtual ~Report();
 
     /**

gui/resultstree.h

@@ -47,7 +47,7 @@ class QItemSelectionModel;
 class ResultsTree : public QTreeView {
     Q_OBJECT
 public:
-    ResultsTree(QWidget * parent = 0);
+    explicit ResultsTree(QWidget * parent = 0);
     virtual ~ResultsTree();
     void Initialize(QSettings *settings, ApplicationList *list);
 

gui/resultsview.h

@@ -43,7 +43,7 @@ class ResultsView : public QWidget {
     Q_OBJECT
 public:
 
-    ResultsView(QWidget * parent = 0);
+    explicit ResultsView(QWidget * parent = 0);
     void Initialize(QSettings *settings, ApplicationList *list);
     virtual ~ResultsView();
 

gui/scratchpad.h

@@ -33,7 +33,7 @@ class MainWindow;
 class ScratchPad : public QDialog {
     Q_OBJECT
 public:
-    ScratchPad(MainWindow& mainWindow);
+    explicit ScratchPad(MainWindow& mainWindow);
 
 private slots:
     /**

gui/statsdialog.h

@@ -35,7 +35,7 @@ class CheckStatistics;
 class StatsDialog : public QDialog {
     Q_OBJECT
 public:
-    StatsDialog(QWidget *parent = 0);
+    explicit StatsDialog(QWidget *parent = 0);
 
     /**
     * @brief Sets the project to extract statistics from

gui/threadhandler.h

@@ -42,7 +42,7 @@ class Settings;
 class ThreadHandler : public QObject {
     Q_OBJECT
 public:
-    ThreadHandler(QObject *parent = 0);
+    explicit ThreadHandler(QObject *parent = 0);
     virtual ~ThreadHandler();
 
     /**

gui/translationhandler.h

@@ -61,7 +61,7 @@ struct TranslationInfo {
 class TranslationHandler : QObject {
     Q_OBJECT
 public:
-    TranslationHandler(QObject *parent = 0);
+    explicit TranslationHandler(QObject *parent = 0);
     virtual ~TranslationHandler();
 
     /**

gui/txtreport.h

@@ -35,7 +35,7 @@ class TxtReport : public Report {
     Q_OBJECT
 
 public:
-    TxtReport(const QString &filename);
+    explicit TxtReport(const QString &filename);
     virtual ~TxtReport();
 
     /**

gui/xmlreport.h

@@ -35,7 +35,7 @@ class QObject;
 */
 class XmlReport : public Report {
 public:
-    XmlReport(const QString &filename);
+    explicit XmlReport(const QString &filename);
 
     /**
      * @brief Read contents of the report file.

gui/xmlreportv1.h

@@ -35,7 +35,7 @@
 */
 class XmlReportV1 : public XmlReport {
 public:
-    XmlReportV1(const QString &filename);
+    explicit XmlReportV1(const QString &filename);
     virtual ~XmlReportV1();
 
     /**

gui/xmlreportv2.h

@@ -36,7 +36,7 @@
 */
 class XmlReportV2 : public XmlReport {
 public:
-    XmlReportV2(const QString &filename);
+    explicit XmlReportV2(const QString &filename);
     virtual ~XmlReportV2();
 
     /**

lib/check.h

@@ -129,7 +129,7 @@ private:
 
     /** disabled assignment operator and copy constructor */
     void operator=(const Check &);
-    Check(const Check &);
+    explicit Check(const Check &);
 };
 
 /// @}

lib/checkclass.cpp

@@ -217,6 +217,52 @@ void CheckClass::constructors()
     }
 }
 
+void CheckClass::checkExplicitConstructors()
+{
+    if (!_settings->isEnabled("style"))
+        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<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
+            if (func->isPure()) {
+                isAbstractClass = true;
+                break;
+            }
+        }
+
+        for (std::list<Function>::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,20 @@ 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)
+{
+    const std::string message(std::string(isStruct ? "Struct" : "Class") + " '" + classname + "' has a constructor with 1 argument that is not explicit.");
+    const std::string verbose(message + " Such constructors should in general be explicit for type safety reasons. Using the explicit keyword in the constructor means some mistakes when using the class can be avoided.");
+    reportError(tok, Severity::style, "noExplicitConstructor", message + "\n" + verbose);
+}
+
+void CheckClass::noExplicitCopyMoveConstructorError(const Token *tok, const std::string &classname, bool isStruct)
+{
+    const std::string message(std::string(isStruct ? "Abstract struct" : "Abstract class") + " '" + classname + "' has a copy/move constructor that is not explicit.");
+    const std::string verbose(message + " For abstract classes, even copy/move constructors may be declared explicit, as, by definition, abstract classes cannot be instantiated, and so objects of such type should never be passed by value.");
+    reportError(tok, Severity::style, "noExplicitCopyMoveConstructor", message + "\n" + verbose);
+}
+
 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);

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"

lib/tokenlist.h

@@ -33,7 +33,7 @@ class Settings;
 
 class CPPCHECKLIB TokenList {
 public:
-    TokenList(const Settings* settings);
+    explicit TokenList(const Settings* settings);
     ~TokenList();
 
     void setSettings(const Settings *settings) {

lib/valueflow.cpp

@@ -1567,7 +1567,7 @@ static void valueFlowForLoopSimplify(Token * const bodyStart, const unsigned int
         }
 
         if (Token::Match(tok2, "%oror%|&&")) {
-            const std::map<unsigned int, MathLib::bigint> programMemory(getProgramMemory(tok2->astTop(), varid, value));
+            const std::map<unsigned int, MathLib::bigint> programMemory(getProgramMemory(tok2->astTop(), varid, ValueFlow::Value(value)));
             if ((tok2->str() == "&&" && conditionIsFalse(tok2->astOperand1(), programMemory)) ||
                 (tok2->str() == "||" && conditionIsTrue(tok2->astOperand1(), programMemory))) {
                 // Skip second expression..
@@ -1584,8 +1584,8 @@ static void valueFlowForLoopSimplify(Token * const bodyStart, const unsigned int
             }
 
         }
-        if ((tok2->str() == "&&" && conditionIsFalse(tok2->astOperand1(), getProgramMemory(tok2->astTop(), varid, value))) ||
+        if ((tok2->str() == "&&" && conditionIsFalse(tok2->astOperand1(), getProgramMemory(tok2->astTop(), varid, ValueFlow::Value(value)))) ||
-            (tok2->str() == "||" && conditionIsTrue(tok2->astOperand1(), getProgramMemory(tok2->astTop(), varid, value))))
+            (tok2->str() == "||" && conditionIsTrue(tok2->astOperand1(), getProgramMemory(tok2->astTop(), varid, ValueFlow::Value(value)))))
             break;
 
         else if (Token::simpleMatch(tok2, ") {") && Token::findmatch(tok2->link(), "%varid%", tok2, varid)) {
@@ -1630,7 +1630,7 @@ static void valueFlowForLoopSimplifyAfter(Token *fortok, unsigned int varid, con
         endToken = fortok->scope()->classEnd;
 
     std::list<ValueFlow::Value> values;
-    values.push_back(num);
+    values.push_back(ValueFlow::Value(num));
 
     valueFlowForward(fortok->linkAt(1)->linkAt(1)->next(),
                      endToken,

lib/valueflow.h

@@ -30,7 +30,7 @@ class Settings;
 namespace ValueFlow {
     class Value {
     public:
-        Value(long long val = 0) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {}
+        explicit Value(long long val = 0) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {}
         Value(const Token *c, long long val) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {}
 
         /** int value */

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("style");
+
+        // 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]: (style) Class 'Class' has a constructor with 1 argument that is not explicit.\n", errout.str());
+
+        checkExplicitConstructors("class Class \n"
+                                  "{ \n"
+                                  "    Class(const Class& other) { } \n"
+                                  "    virtual int i() = 0; \n"
+                                  "};");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Abstract class 'Class' has a copy/move constructor that is not explicit.\n", errout.str());
+
+        checkExplicitConstructors("class Class \n"
+                                  "{ \n"
+                                  "    Class(Class&& other) { } \n"
+                                  "    virtual int i() = 0; \n"
+                                  "};");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Abstract class 'Class' has a copy/move constructor that is not explicit.\n", errout.str());
     }
 
     void checkDuplInheritedMembers(const char code[]) {

test/testsuite.h

@@ -74,7 +74,7 @@ public:
     void warn(const char msg[]) const;
     void warnUnsimplified(const std::string& unsimplified, const std::string& simplified);
 
-    TestFixture(const std::string &_name);
+    explicit TestFixture(const std::string &_name);
     virtual ~TestFixture() { }
 
     static std::size_t runTests(const options& args);