2669: Fix some deficiencies found by cppcheck (TrenchBroom#2670)

* 2669: Fix initialization of default values for attribute definitions by using std::optional. * 2669: Don’t delete iterator delegate on self assignment. * 2669: Initialize file serializer formats in initialization list. * 2669: Add forgotten std::move. * 2669: Inline variable declarations are reported as syntax errors by cppcheck. * 2669: Initialize member in initialization list. * 2669: Remove unused member variable. * 2669: Typos. * 2669: Pass by reference and use of explicit types to avoid a warning. * 2669: More syntax errors. * 2669: Forgotten std::move. * 2669: Use Xcode 10 on travis to enable use of optional header. * 2669: Initialize current object. * 2669: Pass msg by reference. * 2669: Silence a cppcheck warning about using a non const method in an assertion. * 2669: Fix type error. * 2669: Fix auto type oddities. * 2669: Don’t call non const functions in assertions. Be more resilient in these cases and don’t assert at all. * 2669: Fix shortcut sorting. * 2669: Don't hide raw C pointers behind auto. * 2669: Run cppcheck if installed * 2669: Fix more cppcheck warnings. * 2669: Install cppcheck on our travis Linux builds * 2669: Install cppcheck on our travis mac builds * 2669: Skip cppcheck on travis mac, homebrew does not have a cask for it. * 2669: Lies! * 2669: Run cppcheck target specifically, be resilient if cppcheck is not installed. * 2669: Run cppcheck for windows builds too. * 2669: Fix cppcheck cmake integration. * 2669: Enable support for building a report for cppcheck results. Disable cppcheck on travis / linux due to outdated package.
GoaLitiuM · Mar 23, 2019 · f693543 · f693543
1 parent fb4a2b7
commit f693543
Show file tree

Hide file tree

Showing 81 changed files with 476 additions and 378 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -9,9 +9,9 @@ matrix:
       sudo: required
       env: TB_GCC8=true
     - os: osx
-      osx_image: xcode9.3
+      osx_image: xcode10
     - os: osx
-      osx_image: xcode9.3
+      osx_image: xcode10
       env: TB_DEBUG_BUILD=true
 cache:
   directories:

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -127,6 +127,7 @@ INCLUDE(cmake/vecmath.cmake)
 INCLUDE(cmake/Common.cmake)
 INCLUDE(cmake/StackWalker.cmake)
 INCLUDE(cmake/miniz.cmake)
+INCLUDE(cmake/CppCheck.cmake)
 
 INCLUDE(cmake/TrenchBroomApp.cmake)
 INCLUDE(cmake/TrenchBroomTest.cmake)
diff --git a/appveyor.bat b/appveyor.bat
@@ -1,5 +1,5 @@
 PATH=%PATH%;c:\wxWidgets-3.1.1\lib\vc141_dll
-PATH=%PATH%;C:\Program Files (x86)\Pandoc
+PATH=%PATH%;C:\Program Files (x86)\Pandoc;C:\Program Files\Cppcheck
 SET WXWIN="c:\wxWidgets-3.1.1"
 
 mkdir cmakebuild
@@ -9,6 +9,10 @@ cmake .. -G"Visual Studio 15 2017" -T v141_xp -DCMAKE_BUILD_TYPE=Release -DCMAKE
 
 IF ERRORLEVEL 1 GOTO ERROR
 
+cmake --build . --target cppcheck
+
+IF ERRORLEVEL 1 GOTO ERROR
+
 cmake --build . --config Release
 
 IF ERRORLEVEL 1 GOTO ERROR

diff --git a/appveyor.yml b/appveyor.yml
@@ -2,6 +2,7 @@ image: Visual Studio 2017
 version: 2.0.0-appveyor-{build}
 install:
 - cinst pandoc
+- cinst cppcheck
 - ps: Invoke-WebRequest 'https://github.com/wxWidgets/wxWidgets/releases/download/v3.1.1/wxWidgets-3.1.1-headers.7z' -OutFile 'headers.7z'
 - ps: Invoke-WebRequest 'https://github.com/wxWidgets/wxWidgets/releases/download/v3.1.1/wxMSW-3.1.1_vc141_Dev.7z' -OutFile 'dev.7z'
 - ps: Invoke-WebRequest 'https://github.com/wxWidgets/wxWidgets/releases/download/v3.1.1/wxMSW-3.1.1_vc141_ReleaseDLL.7z' -OutFile 'dll.7z'

diff --git a/cmake/CppCheck.cmake b/cmake/CppCheck.cmake
@@ -0,0 +1,45 @@
+LIST(APPEND CPPCHECK_ARGS
+    --enable=warning,performance,portability
+    --verbose
+    --suppressions-list=${CMAKE_SOURCE_DIR}/cppcheck.suppr
+    --std=c++11
+    --language=c++
+    --xml
+    --error-exitcode=1
+    -DMAIN=main
+    -I ${VECMATH_INCLUDE_DIR}
+    ${COMMON_SOURCE_DIR}
+    2> ./err.xml
+)
+
+LIST(APPEND CPPCHECK_HTMLREPORT_ARGS
+    --file err.xml 
+    --report-dir=cppcheck_report 
+    --source-dir=${CMAKE_SOURCE_DIR}
+)
+
+FIND_PROGRAM(CPPCHECK_EXE cppcheck)
+
+IF (CPPCHECK_EXE STREQUAL "CPPCHECK_EXE-NOTFOUND")
+    MESSAGE(STATUS "Could not find cppcheck, skipping checks")
+    ADD_CUSTOM_TARGET(
+        cppcheck
+        COMMENT "skipping cppcheck"
+    )
+ELSE()
+    MESSAGE(STATUS "Using cppcheck found at ${CPPCHECK_EXE}")
+    ADD_CUSTOM_TARGET(
+        cppcheck
+        COMMAND ${CPPCHECK_EXE} ${CPPCHECK_ARGS}
+        COMMENT "running cppcheck"
+    )
+
+    FIND_PROGRAM(CPPCHECK_HTMLREPORT_EXE cppcheck-htmlreport)
+    IF (NOT CPPCHECK_HTMLREPORT_EXE STREQUAL "CPPCHECK_HTMLREPORT_EXE-NOTFOUND")
+        ADD_CUSTOM_TARGET(
+            cppcheck-report
+            COMMAND ${CPPCHECK_HTMLREPORT_EXE} ${CPPCHECK_HTMLREPORT_ARGS}
+            COMMENT "running cppcheck-htmlreport"
+        )
+    ENDIF()
+ENDIF()
diff --git a/common/src/Assets/AttributeDefinition.cpp b/common/src/Assets/AttributeDefinition.cpp
@@ -127,60 +127,32 @@ namespace TrenchBroom {
             return new AttributeDefinition(name, type(), shortDescription, longDescription, readOnly);
         }
 
-        StringAttributeDefinition::StringAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const String& defaultValue, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_StringAttribute, shortDescription, longDescription, defaultValue, readOnly) {}
-
-        StringAttributeDefinition::StringAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_StringAttribute, shortDescription, longDescription, readOnly) {}
+        StringAttributeDefinition::StringAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly, std::optional<String> defaultValue) :
+        AttributeDefinitionWithDefaultValue(name, Type_StringAttribute, shortDescription, longDescription, readOnly, std::move(defaultValue)) {}
 
         AttributeDefinition* StringAttributeDefinition::doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const {
-            if (hasDefaultValue()) {
-                return new StringAttributeDefinition(name, shortDescription, longDescription, defaultValue(), readOnly);
-            } else {
-                return new StringAttributeDefinition(name, shortDescription, longDescription, readOnly);
-            }
+            return new StringAttributeDefinition(name, shortDescription, longDescription, readOnly, m_defaultValue);
         }
 
-        BooleanAttributeDefinition::BooleanAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool defaultValue, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_BooleanAttribute, shortDescription, longDescription, defaultValue, readOnly) {}
-
-        BooleanAttributeDefinition::BooleanAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_BooleanAttribute, shortDescription, longDescription, readOnly) {}
+        BooleanAttributeDefinition::BooleanAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly, std::optional<bool> defaultValue) :
+        AttributeDefinitionWithDefaultValue(name, Type_BooleanAttribute, shortDescription, longDescription, readOnly, std::move(defaultValue)) {}
 
         AttributeDefinition* BooleanAttributeDefinition::doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const {
-            if (hasDefaultValue()) {
-                return new BooleanAttributeDefinition(name, shortDescription, longDescription, defaultValue(), readOnly);
-            } else {
-                return new BooleanAttributeDefinition(name, shortDescription, longDescription, readOnly);
-            }
+            return new BooleanAttributeDefinition(name, shortDescription, longDescription, readOnly, m_defaultValue);
         }
 
-        IntegerAttributeDefinition::IntegerAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const int defaultValue, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_IntegerAttribute, shortDescription, longDescription, defaultValue, readOnly) {}
-
-        IntegerAttributeDefinition::IntegerAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_IntegerAttribute, shortDescription, longDescription, readOnly) {}
+        IntegerAttributeDefinition::IntegerAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly, std::optional<int> defaultValue) :
+        AttributeDefinitionWithDefaultValue(name, Type_IntegerAttribute, shortDescription, longDescription, readOnly, std::move(defaultValue)) {}
 
         AttributeDefinition* IntegerAttributeDefinition::doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const {
-            if (hasDefaultValue()) {
-                return new IntegerAttributeDefinition(name, shortDescription, longDescription, defaultValue(), readOnly);
-            } else {
-                return new IntegerAttributeDefinition(name, shortDescription, longDescription, readOnly);
-            }
+            return new IntegerAttributeDefinition(name, shortDescription, longDescription, readOnly, m_defaultValue);
         }
 
-        FloatAttributeDefinition::FloatAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const float defaultValue, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_FloatAttribute, shortDescription, longDescription, defaultValue, readOnly) {}
-
-        FloatAttributeDefinition::FloatAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_FloatAttribute, shortDescription, longDescription, readOnly) {}
+        FloatAttributeDefinition::FloatAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly, std::optional<float> defaultValue) :
+        AttributeDefinitionWithDefaultValue(name, Type_FloatAttribute, shortDescription, longDescription, readOnly, std::move(defaultValue)) {}
 
         AttributeDefinition* FloatAttributeDefinition::doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const {
-            if (hasDefaultValue()) {
-                return new FloatAttributeDefinition(name, shortDescription, longDescription, defaultValue(), readOnly);
-            } else {
-                return new FloatAttributeDefinition(name, shortDescription, longDescription, readOnly);
-            }
+            return new FloatAttributeDefinition(name, shortDescription, longDescription, readOnly, m_defaultValue);
         }
 
         ChoiceAttributeOption::ChoiceAttributeOption(const String& value, const String& description) :
@@ -199,12 +171,8 @@ namespace TrenchBroom {
             return m_description;
         }
 
-        ChoiceAttributeDefinition::ChoiceAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const ChoiceAttributeOption::List& options, const String& defaultValue, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_ChoiceAttribute, shortDescription, longDescription, defaultValue, readOnly),
-        m_options(options) {}
-
-        ChoiceAttributeDefinition::ChoiceAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const ChoiceAttributeOption::List& options, const bool readOnly) :
-        AttributeDefinitionWithDefaultValue(name, Type_ChoiceAttribute, shortDescription, longDescription, readOnly),
+        ChoiceAttributeDefinition::ChoiceAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const ChoiceAttributeOption::List& options, const bool readOnly, std::optional<String> defaultValue) :
+        AttributeDefinitionWithDefaultValue(name, Type_ChoiceAttribute, shortDescription, longDescription, readOnly, std::move(defaultValue)),
         m_options(options) {}
 
         const ChoiceAttributeOption::List& ChoiceAttributeDefinition::options() const {
@@ -216,11 +184,7 @@ namespace TrenchBroom {
         }
 
         AttributeDefinition* ChoiceAttributeDefinition::doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const {
-            if (hasDefaultValue()) {
-                return new ChoiceAttributeDefinition(name, shortDescription, longDescription, options(), defaultValue(), readOnly);
-            } else {
-                return new ChoiceAttributeDefinition(name, shortDescription, longDescription, options(), readOnly);
-            }
+            return new ChoiceAttributeDefinition(name, shortDescription, longDescription, options(), readOnly, m_defaultValue);
         }
 
         FlagsAttributeOption::FlagsAttributeOption(const int value, const String& shortDescription, const String& longDescription, const bool isDefault) :
@@ -296,19 +260,12 @@ namespace TrenchBroom {
             return result.release();
         }
 
-        UnknownAttributeDefinition::UnknownAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const String& defaultValue, const bool readOnly) :
-        StringAttributeDefinition(name, shortDescription, longDescription, defaultValue, readOnly) {}
-
-        UnknownAttributeDefinition::UnknownAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly) :
-        StringAttributeDefinition(name, shortDescription, longDescription, readOnly) {}
+        UnknownAttributeDefinition::UnknownAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const bool readOnly, std::optional<String> defaultValue) :
+        StringAttributeDefinition(name, shortDescription, longDescription, readOnly, std::move(defaultValue)) {}
 
 
         AttributeDefinition* UnknownAttributeDefinition::doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const {
-            if (hasDefaultValue()) {
-                return new UnknownAttributeDefinition(name, shortDescription, longDescription, defaultValue(), readOnly);
-            } else {
-                return new UnknownAttributeDefinition(name, shortDescription, longDescription, readOnly);
-            }
+            return new UnknownAttributeDefinition(name, shortDescription, longDescription, readOnly, m_defaultValue);
         }
     }
 }
diff --git a/common/src/Assets/AttributeDefinition.h b/common/src/Assets/AttributeDefinition.h
@@ -23,6 +23,7 @@
 #include "StringUtils.h"
 #include "Exceptions.h"
 
+#include <optional>
 #include <vector>
 
 namespace TrenchBroom {
@@ -68,58 +69,50 @@ namespace TrenchBroom {
 
         template <typename T>
         class AttributeDefinitionWithDefaultValue : public AttributeDefinition {
-        private:
-            bool m_hasDefaultValue;
-            T m_defaultValue;
+        protected:
+            std::optional<T> m_defaultValue;
         public:
             bool hasDefaultValue() const {
-                return m_hasDefaultValue;
+                return m_defaultValue.has_value();
             }
 
             const T& defaultValue() const {
-                if (!hasDefaultValue())
+                if (!hasDefaultValue()) {
                     throw EntityAttributeException(name() + " has no default value");
-                return m_defaultValue;
+                } else {
+                    return *m_defaultValue;
+                }
             }
         protected:
-            AttributeDefinitionWithDefaultValue(const String& name, Type type, const String& shortDescription, const String& longDescription, bool readOnly) :
-            AttributeDefinition(name, type, shortDescription, longDescription, readOnly),
-            m_hasDefaultValue(false) {}
-
-            AttributeDefinitionWithDefaultValue(const String& name, Type type, const String& shortDescription, const String& longDescription, const T& defaultValue, bool readOnly) :
+            AttributeDefinitionWithDefaultValue(const String& name, Type type, const String& shortDescription, const String& longDescription, bool readOnly, std::optional<T> defaultValue = std::nullopt) :
             AttributeDefinition(name, type, shortDescription, longDescription, readOnly),
-            m_hasDefaultValue(true),
-            m_defaultValue(defaultValue) {}
+            m_defaultValue(std::move(defaultValue)) {}
         };
 
         class StringAttributeDefinition : public AttributeDefinitionWithDefaultValue<String> {
         public:
-            StringAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const String& defaultValue, bool readOnly);
-            StringAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly);
+            StringAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly, std::optional<String> defaultValue = std::nullopt);
         private:
             AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override;
         };
 
         class BooleanAttributeDefinition : public AttributeDefinitionWithDefaultValue<bool> {
         public:
-            BooleanAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool defaultValue, bool readOnly);
-            BooleanAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly);
+            BooleanAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly, std::optional<bool> defaultValue = std::nullopt);
         private:
             AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override;
         };
 
         class IntegerAttributeDefinition : public AttributeDefinitionWithDefaultValue<int> {
         public:
-            IntegerAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, int defaultValue, bool readOnly);
-            IntegerAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly);
+            IntegerAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly, std::optional<int> defaultValue = std::nullopt);
         private:
             AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override;
         };
 
         class FloatAttributeDefinition : public AttributeDefinitionWithDefaultValue<float> {
         public:
-            FloatAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, float defaultValue, bool readOnly);
-            FloatAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly);
+            FloatAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly, std::optional<float> defaultValue = std::nullopt);
         private:
             AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override;
         };
@@ -141,8 +134,7 @@ namespace TrenchBroom {
         private:
             ChoiceAttributeOption::List m_options;
         public:
-            ChoiceAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const ChoiceAttributeOption::List& options, const String& defaultValue, bool readOnly);
-            ChoiceAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const ChoiceAttributeOption::List& options, bool readOnly);
+            ChoiceAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const ChoiceAttributeOption::List& options, bool readOnly, std::optional<String> defaultValue = std::nullopt);
             const ChoiceAttributeOption::List& options() const;
         private:
             bool doEquals(const AttributeDefinition* other) const override;
@@ -183,8 +175,7 @@ namespace TrenchBroom {
 
         class UnknownAttributeDefinition : public StringAttributeDefinition {
         public:
-            UnknownAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, const String& defaultValue, bool readOnly);
-            UnknownAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly);
+            UnknownAttributeDefinition(const String& name, const String& shortDescription, const String& longDescription, bool readOnly, std::optional<String> defaultValue = std::nullopt);
         private:
             AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override;
         };