From f69354399082d2692f39af13e71d46166304d8cb Mon Sep 17 00:00:00 2001 From: Kristian Duske Date: Sat, 23 Mar 2019 18:59:04 +0100 Subject: [PATCH] 2669: Fix some deficiencies found by cppcheck (#2670) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .travis.yml | 4 +- CMakeLists.txt | 1 + appveyor.bat | 6 +- appveyor.yml | 1 + cmake/CppCheck.cmake | 45 +++++++++ common/src/Assets/AttributeDefinition.cpp | 79 ++++------------ common/src/Assets/AttributeDefinition.h | 41 ++++---- common/src/DoublyLinkedList.h | 55 +++++------ common/src/Exceptions.cpp | 4 +- common/src/Exceptions.h | 6 +- common/src/IO/EntParser.cpp | 94 ++++++++----------- common/src/IO/EntParser.h | 7 +- common/src/IO/FgdParser.cpp | 69 +++++--------- common/src/IO/FgdParser.h | 18 +--- common/src/IO/MapFileSerializer.cpp | 20 ++-- common/src/IO/ObjSerializer.cpp | 3 +- common/src/IO/ObjSerializer.h | 2 +- common/src/IO/Tokenizer.h | 29 +++--- common/src/IO/ZipFileSystem.cpp | 3 +- common/src/Model/Tag.cpp | 4 +- common/src/Renderer/BrushRenderer.cpp | 6 +- common/src/Renderer/BrushRendererArrays.cpp | 11 ++- common/src/Renderer/Renderable.cpp | 2 - common/src/Renderer/Renderable.h | 4 +- common/src/Renderer/ShaderProgram.cpp | 6 +- common/src/Renderer/Vbo.cpp | 64 +++++++------ common/src/TemporarilySetAny.h | 1 - common/src/View/CameraTool2D.cpp | 4 + common/src/View/CameraTool2D.h | 3 +- common/src/View/CameraTool3D.cpp | 4 + common/src/View/CameraTool3D.h | 1 + common/src/View/ClipToolController.cpp | 12 +++ common/src/View/ClipToolController.h | 4 + .../CreateComplexBrushToolController3D.cpp | 6 ++ .../View/CreateComplexBrushToolController3D.h | 1 + .../src/View/CreateEntityToolController.cpp | 4 + common/src/View/CreateEntityToolController.h | 1 + .../CreateSimpleBrushToolController2D.cpp | 4 + .../View/CreateSimpleBrushToolController2D.h | 1 + .../CreateSimpleBrushToolController3D.cpp | 4 + .../View/CreateSimpleBrushToolController3D.h | 1 + common/src/View/KeyboardShortcut.cpp | 6 -- common/src/View/KeyboardShortcutGridTable.cpp | 15 ++- common/src/View/MoveObjectsToolController.cpp | 4 + common/src/View/MoveObjectsToolController.h | 1 + .../src/View/ResizeBrushesToolController.cpp | 4 + common/src/View/ResizeBrushesToolController.h | 1 + .../src/View/RotateObjectsToolController.cpp | 12 +++ common/src/View/RotateObjectsToolController.h | 1 + common/src/View/ScaleObjectsTool.cpp | 6 +- common/src/View/ScaleObjectsTool.h | 2 +- .../src/View/ScaleObjectsToolController.cpp | 7 +- common/src/View/ScaleObjectsToolController.h | 1 + common/src/View/SelectionTool.cpp | 4 + common/src/View/SelectionTool.h | 1 + .../src/View/SetBrushFaceAttributesTool.cpp | 4 + common/src/View/SetBrushFaceAttributesTool.h | 1 + .../src/View/ShearObjectsToolController.cpp | 4 + common/src/View/ShearObjectsToolController.h | 1 + common/src/View/ToolController.cpp | 3 +- common/src/View/ToolController.h | 4 +- common/src/View/UVCameraTool.cpp | 4 + common/src/View/UVCameraTool.h | 1 + common/src/View/UVOffsetTool.cpp | 4 + common/src/View/UVOffsetTool.h | 1 + common/src/View/UVOriginTool.cpp | 4 + common/src/View/UVOriginTool.h | 1 + common/src/View/UVRotateTool.cpp | 4 + common/src/View/UVRotateTool.h | 1 + common/src/View/UVScaleTool.cpp | 4 + common/src/View/UVScaleTool.h | 1 + common/src/View/UVShearTool.cpp | 4 + common/src/View/UVShearTool.h | 1 + common/src/View/VertexToolControllerBase.h | 12 +++ cppcheck.suppr | 10 ++ test/src/Renderer/VertexTest.cpp | 4 +- test/src/View/MoveToolControllerTest.cpp | 1 + test/src/vec_test.cpp | 4 - travis-linux.sh | 4 +- travis-macos.sh | 3 +- vecmath/include/vecmath/vec.h | 69 ++++++-------- 81 files changed, 476 insertions(+), 378 deletions(-) create mode 100644 cmake/CppCheck.cmake create mode 100644 cppcheck.suppr diff --git a/.travis.yml b/.travis.yml index 8a25d3e27f..9c790c8c7b 100644 --- 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 index 7a1dd31f36..31ec89de8a 100644 --- 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 index e948ca8328..7e1befa2d0 100644 --- 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 index 4500648e05..ceb932f91b 100644 --- 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 new file mode 100644 index 0000000000..25297b4309 --- /dev/null +++ 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 index 4863acc1fa..1aecc456bc 100644 --- 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 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 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 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 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 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 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 index b97514d0ee..0479e1b2e8 100644 --- a/common/src/Assets/AttributeDefinition.h +++ b/common/src/Assets/AttributeDefinition.h @@ -23,6 +23,7 @@ #include "StringUtils.h" #include "Exceptions.h" +#include #include namespace TrenchBroom { @@ -68,58 +69,50 @@ namespace TrenchBroom { template class AttributeDefinitionWithDefaultValue : public AttributeDefinition { - private: - bool m_hasDefaultValue; - T m_defaultValue; + protected: + std::optional 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 defaultValue = std::nullopt) : AttributeDefinition(name, type, shortDescription, longDescription, readOnly), - m_hasDefaultValue(true), - m_defaultValue(defaultValue) {} + m_defaultValue(std::move(defaultValue)) {} }; class StringAttributeDefinition : public AttributeDefinitionWithDefaultValue { 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 defaultValue = std::nullopt); private: AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override; }; class BooleanAttributeDefinition : public AttributeDefinitionWithDefaultValue { 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 defaultValue = std::nullopt); private: AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override; }; class IntegerAttributeDefinition : public AttributeDefinitionWithDefaultValue { 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 defaultValue = std::nullopt); private: AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override; }; class FloatAttributeDefinition : public AttributeDefinitionWithDefaultValue { 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 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 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 defaultValue = std::nullopt); private: AttributeDefinition* doClone(const String& name, const String& shortDescription, const String& longDescription, bool readOnly) const override; }; diff --git a/common/src/DoublyLinkedList.h b/common/src/DoublyLinkedList.h index b978805342..6d68657ae6 100644 --- a/common/src/DoublyLinkedList.h +++ b/common/src/DoublyLinkedList.h @@ -34,9 +34,9 @@ class DoublyLinkedList { Item* m_previous; Item* m_next; public: - Link(Item* item): - m_previous(item), - m_next(item) { + explicit Link(Item* item): + m_previous(item), + m_next(item) { ensure(item != nullptr, "item is null"); } @@ -58,14 +58,6 @@ class DoublyLinkedList { m_next = next; } - bool selfLoop(Item* item) const { - return m_previous == item && m_next == item; - } - - void unlink(Item* item) { - m_previous = m_next = item; - } - void flip() { using std::swap; swap(m_previous, m_next); @@ -86,8 +78,8 @@ class DoublyLinkedList { size_t m_listVersion; protected: explicit iterator_delegate_base(ListType& list) : - m_list(&list), - m_listVersion(m_list->m_version) {} + m_list(&list), + m_listVersion(m_list->m_version) {} public: virtual ~iterator_delegate_base() = default; public: @@ -122,9 +114,9 @@ class DoublyLinkedList { size_t m_index; public: iterator_delegate_item(ListType& list, ItemType item, const size_t index) : - base(list), - m_item(item), - m_index(index) {} + base(list), + m_item(item), + m_index(index) {} private: base* doClone() const override { return new iterator_delegate_item(*base::m_list, m_item, m_index); @@ -153,7 +145,7 @@ class DoublyLinkedList { using base = iterator_delegate_base; public: explicit iterator_delegate_end(ListType& list) : - base(list) {} + base(list) {} private: base* doClone() const override { return new iterator_delegate_end(*base::m_list); @@ -202,8 +194,10 @@ class DoublyLinkedList { } iterator_base& operator=(const iterator_base& other) { - delete m_delegate; - m_delegate = other.m_delegate->clone(); + if (this != &other) { + delete m_delegate; + m_delegate = other.m_delegate->clone(); + } return *this; } @@ -219,7 +213,7 @@ class DoublyLinkedList { } // postfix increment - iterator_base operator++(int) { + const iterator_base operator++(int) { iterator_base result(*this); m_delegate->increment(); return result; @@ -251,16 +245,16 @@ class DoublyLinkedList { size_t m_version; public: DoublyLinkedList() : - m_getLink(), - m_head(nullptr), - m_size(0), - m_version(0) {} - - DoublyLinkedList(DoublyLinkedList&& other) : - m_getLink(std::move(other.m_getLink)), - m_head(other.m_head), - m_size(other.m_size), - m_version(other.m_version) { + m_getLink(), + m_head(nullptr), + m_size(0), + m_version(0) {} + + DoublyLinkedList(DoublyLinkedList&& other) noexcept : + m_getLink(std::move(other.m_getLink)), + m_head(other.m_head), + m_size(other.m_size), + m_version(other.m_version) { other.m_head = nullptr; other.m_size = 0; other.m_version += 1; @@ -286,6 +280,7 @@ class DoublyLinkedList { other.m_head = nullptr; other.m_size = 0; other.m_version += 1; + return *this; } public: // Copying is not allowed since this is an intrusive list. diff --git a/common/src/Exceptions.cpp b/common/src/Exceptions.cpp index 06a2db9556..1ffccd8008 100644 --- a/common/src/Exceptions.cpp +++ b/common/src/Exceptions.cpp @@ -21,8 +21,8 @@ Exception::Exception() noexcept {} -Exception::Exception(std::string str) noexcept : -m_msg(std::move(str)) {} +Exception::Exception(const std::string& str) noexcept : +m_msg(str) {} const char* Exception::what() const noexcept { return m_msg.c_str(); diff --git a/common/src/Exceptions.h b/common/src/Exceptions.h index fed84e2f3f..52ea3881cf 100644 --- a/common/src/Exceptions.h +++ b/common/src/Exceptions.h @@ -31,7 +31,7 @@ class Exception : public std::exception { std::string m_msg; public: Exception() noexcept; - explicit Exception(std::string str) noexcept; + explicit Exception(const std::string& str) noexcept; const char* what() const noexcept override; }; @@ -42,8 +42,8 @@ class ExceptionStream : public Exception { ExceptionStream() noexcept : Exception() {} - explicit ExceptionStream(std::string str) noexcept : - Exception(std::move(str)) {} + explicit ExceptionStream(const std::string& str) noexcept : + Exception(str) {} template C& operator<< (T value) { diff --git a/common/src/IO/EntParser.cpp b/common/src/IO/EntParser.cpp index 718f1d50d6..f5bd6a6b72 100644 --- a/common/src/IO/EntParser.cpp +++ b/common/src/IO/EntParser.cpp @@ -147,12 +147,12 @@ namespace TrenchBroom { if (flagElement != nullptr) { auto result = std::make_shared(Model::AttributeNames::Spawnflags); do { - const auto [success, bit] = parseSize(*flagElement, "bit", status); - if (!success) { + const auto bit = parseSize(*flagElement, "bit", status); + if (!bit.has_value()) { const auto strValue = parseString(*flagElement, "bit", status); warn(*flagElement, "Invalid value '" + strValue + "' for bit attribute", status); } else { - const auto value = 1 << bit; + const auto value = 1 << *bit; const auto shortDesc = parseString(element, "key", status); const auto longDesc = parseString(element, "name", status); result->addOption(value, shortDesc, longDesc, false); @@ -209,24 +209,16 @@ namespace TrenchBroom { void EntParser::parseUnknownAttribute(const tinyxml2::XMLElement& element, Assets::AttributeDefinitionList& attributeDefinitions, ParserStatus& status) { auto factory = [this, &element, &status](const String& name, const String& shortDesc, const String& longDesc) { - if (hasAttribute(element, "value")) { - const auto value = parseString(element, "value", status); - return std::make_shared(name, shortDesc, longDesc, value, false); - } else { - return std::make_shared(name, shortDesc, longDesc, false); - } + auto defaultValue = hasAttribute(element, "value") ? std::make_optional(parseString(element, "value", status)) : std::nullopt; + return std::make_shared(name, shortDesc, longDesc, false, std::move(defaultValue)); }; parseAttributeDefinition(element, factory, attributeDefinitions, status); } void EntParser::parseStringAttribute(const tinyxml2::XMLElement& element, Assets::AttributeDefinitionList& attributeDefinitions, ParserStatus& status) { auto factory = [this, &element, &status](const String& name, const String& shortDesc, const String& longDesc) { - if (hasAttribute(element, "value")) { - const auto value = parseString(element, "value", status); - return std::make_shared(name, shortDesc, longDesc, value, false); - } else { - return std::make_shared(name, shortDesc, longDesc, false); - } + auto defaultValue = hasAttribute(element, "value") ? std::make_optional(parseString(element, "value", status)) : std::nullopt; + return std::make_shared(name, shortDesc, longDesc, false, std::move(defaultValue)); }; parseAttributeDefinition(element, factory, attributeDefinitions, status); } @@ -234,13 +226,13 @@ namespace TrenchBroom { void EntParser::parseBooleanAttribute(const tinyxml2::XMLElement& element, Assets::AttributeDefinitionList& attributeDefinitions, ParserStatus& status) { auto factory = [this, &element, &status](const String& name, const String& shortDesc, const String& longDesc) -> Assets::AttributeDefinitionPtr { if (hasAttribute(element, "value")) { - const auto [success, value] = parseInteger(element, "value", status); - if (success) { - return std::make_shared(name, shortDesc, longDesc, value != 0, false); + const auto boolDefaultValue = parseInteger(element, "value", status); + if (boolDefaultValue.has_value()) { + return std::make_shared(name, shortDesc, longDesc, false, *boolDefaultValue != 0); } else { - const auto strValue = parseString(element, "value", status); - warn(element, "Invalid default value '" + strValue + "' for boolean attribute definition", status); - return std::make_shared(name, shortDesc, longDesc, strValue, false); + auto strDefaultValue = parseString(element, "value", status); + warn(element, "Invalid default value '" + strDefaultValue + "' for boolean attribute definition", status); + return std::make_shared(name, shortDesc, longDesc, false, std::move(strDefaultValue)); } } else { return std::make_shared(name, shortDesc, longDesc, false); @@ -252,13 +244,13 @@ namespace TrenchBroom { void EntParser::parseIntegerAttribute(const tinyxml2::XMLElement& element, Assets::AttributeDefinitionList& attributeDefinitions, ParserStatus& status) { auto factory = [this, &element, &status](const String& name, const String& shortDesc, const String& longDesc) -> Assets::AttributeDefinitionPtr { if (hasAttribute(element, "value")) { - const auto [success, value] = parseInteger(element, "value", status); - if (success) { - return std::make_shared(name, shortDesc, longDesc, value, false); + auto intDefaultValue = parseInteger(element, "value", status); + if (intDefaultValue.has_value()) { + return std::make_shared(name, shortDesc, longDesc, false, std::move(intDefaultValue)); } else { - const auto strValue = parseString(element, "value", status); - warn(element, "Invalid default value '" + strValue + "' for integer attribute definition", status); - return std::make_shared(name, shortDesc, longDesc, strValue, false); + auto strDefaultValue = parseString(element, "value", status); + warn(element, "Invalid default value '" + strDefaultValue + "' for integer attribute definition", status); + return std::make_shared(name, shortDesc, longDesc, false, std::move(strDefaultValue)); } } else { return std::make_shared(name, shortDesc, longDesc, false); @@ -270,13 +262,13 @@ namespace TrenchBroom { void EntParser::parseRealAttribute(const tinyxml2::XMLElement& element, Assets::AttributeDefinitionList& attributeDefinitions, ParserStatus& status) { auto factory = [this, &element, &status](const String& name, const String& shortDesc, const String& longDesc) -> Assets::AttributeDefinitionPtr { if (hasAttribute(element, "value")) { - const auto [success, value] = parseFloat(element, "value", status); - if (success) { - return std::make_shared(name, shortDesc, longDesc, value, false); + auto floatDefaultValue = parseFloat(element, "value", status); + if (floatDefaultValue.has_value()) { + return std::make_shared(name, shortDesc, longDesc, false, std::move(floatDefaultValue)); } else { - const auto strValue = parseString(element, "value", status); - warn(element, "Invalid default value '" + strValue + "' for float attribute definition", status); - return std::make_shared(name, shortDesc, longDesc, strValue, false); + auto strDefaultValue = parseString(element, "value", status); + warn(element, "Invalid default value '" + strDefaultValue + "' for float attribute definition", status); + return std::make_shared(name, shortDesc, longDesc, false, std::move(strDefaultValue)); } } else { return std::make_shared(name, shortDesc, longDesc, false); @@ -358,46 +350,40 @@ namespace TrenchBroom { return Color::parse(parseString(element, attributeName, status)); } - std::tuple EntParser::parseInteger(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { + std::optional EntParser::parseInteger(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { const auto* strValue = element.Attribute(attributeName.c_str()); - if (strValue == nullptr) { - return std::make_tuple(false, 0); - } else { + if (strValue != nullptr) { char* end; const auto intValue = std::strtol(strValue, &end, 10); - if (*end != '\0' || errno == ERANGE) { - return std::make_tuple(false, 0); + if (*end == '\0' && errno != ERANGE) { + return intValue; } - return std::make_tuple(true, intValue); } + return std::nullopt; } - std::tuple EntParser::parseFloat(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { + std::optional EntParser::parseFloat(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { const auto* strValue = element.Attribute(attributeName.c_str()); - if (strValue == nullptr) { - return std::make_tuple(false, 0.0f); - } else { + if (strValue != nullptr) { char* end; const auto floatValue = std::strtof(strValue, &end); - if (*end != '\0' || errno == ERANGE) { - return std::make_tuple(false, 0.0f); + if (*end == '\0' && errno != ERANGE) { + return floatValue; } - return std::make_tuple(true, floatValue); } + return std::nullopt; } - std::tuple EntParser::parseSize(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { + std::optional EntParser::parseSize(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { const auto* strValue = element.Attribute(attributeName.c_str()); - if (strValue == nullptr) { - return std::make_tuple(false, 0); - } else { + if (strValue != nullptr) { char* end; const auto intValue = std::strtoul(strValue, &end, 10); - if (*end != '\0' || errno == ERANGE) { - return std::make_tuple(false, 0); + if (*end == '\0' && errno != ERANGE) { + return static_cast(intValue); } - return std::make_tuple(true, static_cast(intValue)); } + return std::nullopt; } String EntParser::parseString(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status) { diff --git a/common/src/IO/EntParser.h b/common/src/IO/EntParser.h index 1329519f2e..9077190cac 100644 --- a/common/src/IO/EntParser.h +++ b/common/src/IO/EntParser.h @@ -28,6 +28,7 @@ #include #include +#include namespace tinyxml2 { class XMLDocument; @@ -80,9 +81,9 @@ namespace TrenchBroom { vm::bbox3 parseBounds(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); Color parseColor(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); - std::tuple parseInteger(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); - std::tuple parseFloat(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); - std::tuple parseSize(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); + std::optional parseInteger(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); + std::optional parseFloat(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); + std::optional parseSize(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); String parseString(const tinyxml2::XMLElement& element, const String& attributeName, ParserStatus& status); String getText(const tinyxml2::XMLElement& element); diff --git a/common/src/IO/FgdParser.cpp b/common/src/IO/FgdParser.cpp index 45a8ff8859..7af7058c09 100644 --- a/common/src/IO/FgdParser.cpp +++ b/common/src/IO/FgdParser.cpp @@ -461,11 +461,7 @@ namespace TrenchBroom { const auto defaultValue = parseDefaultStringValue(status); const auto longDescription = parseAttributeDescription(status); - if (defaultValue.present) { - return Assets::AttributeDefinitionPtr(new Assets::StringAttributeDefinition(name, shortDescription, longDescription, defaultValue.value, readOnly)); - } else { - return Assets::AttributeDefinitionPtr(new Assets::StringAttributeDefinition(name, shortDescription, longDescription, readOnly)); - } + return std::make_shared(name, shortDescription, longDescription, readOnly, defaultValue); } Assets::AttributeDefinitionPtr FgdParser::parseIntegerAttribute(ParserStatus& status, const String& name) { @@ -474,11 +470,7 @@ namespace TrenchBroom { const auto defaultValue = parseDefaultIntegerValue(status); const auto longDescription = parseAttributeDescription(status); - if (defaultValue.present) { - return Assets::AttributeDefinitionPtr(new Assets::IntegerAttributeDefinition(name, shortDescription, longDescription, defaultValue.value, readOnly)); - } else { - return Assets::AttributeDefinitionPtr(new Assets::IntegerAttributeDefinition(name, shortDescription, longDescription, readOnly)); - } + return std::make_shared(name, shortDescription, longDescription, readOnly, defaultValue); } Assets::AttributeDefinitionPtr FgdParser::parseFloatAttribute(ParserStatus& status, const String& name) { @@ -487,11 +479,7 @@ namespace TrenchBroom { const auto defaultValue = parseDefaultFloatValue(status); const auto longDescription = parseAttributeDescription(status); - if (defaultValue.present) { - return Assets::AttributeDefinitionPtr(new Assets::FloatAttributeDefinition(name, shortDescription, longDescription, defaultValue.value, readOnly)); - } else { - return Assets::AttributeDefinitionPtr(new Assets::FloatAttributeDefinition(name, shortDescription, longDescription, readOnly)); - } + return std::make_shared(name, shortDescription, longDescription, readOnly, defaultValue); } Assets::AttributeDefinitionPtr FgdParser::parseChoicesAttribute(ParserStatus& status, const String& name) { @@ -515,11 +503,7 @@ namespace TrenchBroom { token = expect(status, FgdToken::Integer | FgdToken::Decimal | FgdToken::String | FgdToken::CBracket, m_tokenizer.nextToken()); } - if (defaultValue.present) { - return Assets::AttributeDefinitionPtr(new Assets::ChoiceAttributeDefinition(name, shortDescription, longDescription, options, defaultValue.value, readOnly)); - } else { - return Assets::AttributeDefinitionPtr(new Assets::ChoiceAttributeDefinition(name, shortDescription, longDescription, options, readOnly)); - } + return std::make_shared(name, shortDescription, longDescription, options, readOnly, defaultValue); } Assets::AttributeDefinitionPtr FgdParser::parseFlagsAttribute(ParserStatus& status, const String& name) { @@ -530,7 +514,7 @@ namespace TrenchBroom { auto token = expect(status, FgdToken::Integer | FgdToken::CBracket, m_tokenizer.nextToken()); - auto* definition = new Assets::FlagsAttributeDefinition(name); + auto definition = std::make_shared(name); while (token.type() != FgdToken::CBracket) { const auto value = token.toInteger(); @@ -555,8 +539,7 @@ namespace TrenchBroom { definition->addOption(value, shortDescription, longDescription, defaultValue); } - - return Assets::AttributeDefinitionPtr(definition); + return definition; } Assets::AttributeDefinitionPtr FgdParser::parseUnknownAttribute(ParserStatus& status, const String& name) { @@ -565,11 +548,7 @@ namespace TrenchBroom { const auto defaultValue = parseDefaultStringValue(status); const auto longDescription = parseAttributeDescription(status); - if (defaultValue.present) { - return Assets::AttributeDefinitionPtr(new Assets::UnknownAttributeDefinition(name, shortDescription, longDescription, defaultValue.value, readOnly)); - } else { - return Assets::AttributeDefinitionPtr(new Assets::UnknownAttributeDefinition(name, shortDescription, longDescription, readOnly)); - } + return std::make_shared(name, shortDescription, longDescription, readOnly, defaultValue); } bool FgdParser::parseReadOnlyFlag(ParserStatus& status) { @@ -594,37 +573,37 @@ namespace TrenchBroom { return EmptyString; } - FgdParser::DefaultValue FgdParser::parseDefaultStringValue(ParserStatus& status) { + std::optional FgdParser::parseDefaultStringValue(ParserStatus& status) { auto token = m_tokenizer.peekToken(); if (token.type() == FgdToken::Colon) { m_tokenizer.nextToken(); token = expect(status, FgdToken::String | FgdToken::Colon, m_tokenizer.peekToken()); if (token.type() == FgdToken::String) { token = m_tokenizer.nextToken(); - return DefaultValue(token.data()); + return token.data(); } } - return DefaultValue(); + return std::nullopt; } - FgdParser::DefaultValue FgdParser::parseDefaultIntegerValue(ParserStatus& status) { + std::optional FgdParser::parseDefaultIntegerValue(ParserStatus& status) { auto token = m_tokenizer.peekToken(); if (token.type() == FgdToken::Colon) { m_tokenizer.nextToken(); token = expect(status, FgdToken::Integer | FgdToken::Decimal | FgdToken::Colon, m_tokenizer.peekToken()); if (token.type() == FgdToken::Integer) { token = m_tokenizer.nextToken(); - return DefaultValue(token.toInteger()); + return token.toInteger(); } else if (token.type() == FgdToken::Decimal) { // be graceful for DaZ token = m_tokenizer.nextToken(); status.warn(token.line(), token.column(), "Found float default value for integer property"); - return DefaultValue(static_cast(token.toFloat())); + return static_cast(token.toFloat()); } } - return DefaultValue(); + return std::nullopt; } - FgdParser::DefaultValue FgdParser::parseDefaultFloatValue(ParserStatus& status) { + std::optional FgdParser::parseDefaultFloatValue(ParserStatus& status) { auto token = m_tokenizer.peekToken(); if (token.type() == FgdToken::Colon) { m_tokenizer.nextToken(); @@ -635,29 +614,23 @@ namespace TrenchBroom { if (token.type() != FgdToken::String) { status.warn(token.line(), token.column(), "Unquoted float default value " + token.data()); } - return DefaultValue(token.toFloat()); + return token.toFloat(); } } - return DefaultValue(); + return std::nullopt; } - FgdParser::DefaultValue FgdParser::parseDefaultChoiceValue(ParserStatus& status) { + std::optional FgdParser::parseDefaultChoiceValue(ParserStatus& status) { auto token = m_tokenizer.peekToken(); if (token.type() == FgdToken::Colon) { m_tokenizer.nextToken(); token = expect(status, FgdToken::String | FgdToken::Integer | FgdToken::Decimal | FgdToken::Colon, m_tokenizer.peekToken()); - if (token.hasType(FgdToken::String)) { - token = m_tokenizer.nextToken(); - return DefaultValue(token.data()); - } else if (token.hasType(FgdToken::Integer)) { - token = m_tokenizer.nextToken(); - return DefaultValue(token.data()); - } else if (token.hasType(FgdToken::Decimal)) { + if (token.hasType(FgdToken::String | FgdToken::Integer | FgdToken::Decimal)) { token = m_tokenizer.nextToken(); - return DefaultValue(token.data()); + return token.data(); } } - return DefaultValue(); + return std::nullopt; } vm::vec3 FgdParser::parseVector(ParserStatus& status) { diff --git a/common/src/IO/FgdParser.h b/common/src/IO/FgdParser.h index f113ba57c9..c5783723de 100644 --- a/common/src/IO/FgdParser.h +++ b/common/src/IO/FgdParser.h @@ -34,6 +34,7 @@ #include #include +#include namespace TrenchBroom { namespace IO { @@ -67,15 +68,6 @@ namespace TrenchBroom { private: using Token = FgdTokenizer::Token; - template - struct DefaultValue { - bool present; - T value; - - DefaultValue() : present(false) {} - explicit DefaultValue(const T& i_value) : present(true), value(i_value) {} - }; - Color m_defaultEntityColor; std::list m_paths; @@ -122,10 +114,10 @@ namespace TrenchBroom { bool parseReadOnlyFlag(ParserStatus& status); String parseAttributeDescription(ParserStatus& status); - DefaultValue parseDefaultStringValue(ParserStatus& status); - DefaultValue parseDefaultIntegerValue(ParserStatus& status); - DefaultValue parseDefaultFloatValue(ParserStatus& status); - DefaultValue parseDefaultChoiceValue(ParserStatus& status); + std::optional parseDefaultStringValue(ParserStatus& status); + std::optional parseDefaultIntegerValue(ParserStatus& status); + std::optional parseDefaultFloatValue(ParserStatus& status); + std::optional parseDefaultChoiceValue(ParserStatus& status); vm::vec3 parseVector(ParserStatus& status); vm::bbox3 parseSize(ParserStatus& status); diff --git a/common/src/IO/MapFileSerializer.cpp b/common/src/IO/MapFileSerializer.cpp index c1e2ce0c33..80863e12ba 100644 --- a/common/src/IO/MapFileSerializer.cpp +++ b/common/src/IO/MapFileSerializer.cpp @@ -33,7 +33,11 @@ namespace TrenchBroom { String TextureInfoFormat; public: QuakeFileSerializer(FILE* stream) : - MapFileSerializer(stream) { + MapFileSerializer(stream), + FacePointFormat(getFacePointFormat()), + TextureInfoFormat(" %s %.6g %.6g %.6g %.6g %.6g") {} + private: + static String getFacePointFormat() { StringStream str; str << "( %." << FloatPrecision << "g " << @@ -45,9 +49,7 @@ namespace TrenchBroom { "( %." << FloatPrecision << "g " << "%." << FloatPrecision << "g " << "%." << FloatPrecision << "g )"; - - FacePointFormat = str.str(); - TextureInfoFormat = " %s %.6g %.6g %.6g %.6g %.6g"; + return str.str(); } private: size_t doWriteBrushFace(FILE* stream, Model::BrushFace* face) override { @@ -89,9 +91,8 @@ namespace TrenchBroom { String SurfaceAttributesFormat; public: Quake2FileSerializer(FILE* stream) : - QuakeFileSerializer(stream) { - SurfaceAttributesFormat = " %d %d %.6g"; - } + QuakeFileSerializer(stream), + SurfaceAttributesFormat(" %d %d %.6g") {} private: size_t doWriteBrushFace(FILE* stream, Model::BrushFace* face) override { writeFacePoints(stream, face); @@ -119,9 +120,8 @@ namespace TrenchBroom { String SurfaceColorFormat; public: DaikatanaFileSerializer(FILE* stream) : - Quake2FileSerializer(stream) { - SurfaceColorFormat = " %d %d %d"; - } + Quake2FileSerializer(stream), + SurfaceColorFormat(" %d %d %d") {} private: size_t doWriteBrushFace(FILE* stream, Model::BrushFace* face) override { writeFacePoints(stream, face); diff --git a/common/src/IO/ObjSerializer.cpp b/common/src/IO/ObjSerializer.cpp index 1a7984a2a7..87fad65e63 100644 --- a/common/src/IO/ObjSerializer.cpp +++ b/common/src/IO/ObjSerializer.cpp @@ -34,7 +34,8 @@ namespace TrenchBroom { normal(i_normal) {} ObjFileSerializer::ObjFileSerializer(FILE* stream) : - m_stream(stream) { + m_stream(stream), + m_currentObject({ 0, 0, {} }) { ensure(m_stream != nullptr, "stream is null"); } diff --git a/common/src/IO/ObjSerializer.h b/common/src/IO/ObjSerializer.h index 6528cbab37..c5261040f9 100644 --- a/common/src/IO/ObjSerializer.h +++ b/common/src/IO/ObjSerializer.h @@ -84,7 +84,7 @@ namespace TrenchBroom { Object m_currentObject; ObjectList m_objects; public: - ObjFileSerializer(FILE* stream); + explicit ObjFileSerializer(FILE* stream); private: void doBeginFile() override; void doEndFile() override; diff --git a/common/src/IO/Tokenizer.h b/common/src/IO/Tokenizer.h index 26de9c8a6f..c33c1cd72f 100644 --- a/common/src/IO/Tokenizer.h +++ b/common/src/IO/Tokenizer.h @@ -51,7 +51,7 @@ namespace TrenchBroom { const char* curPos() const; char curChar() const; - char lookAhead(const size_t offset = 1) const; + char lookAhead(size_t offset = 1) const; size_t line() const; size_t column() const; @@ -65,7 +65,7 @@ namespace TrenchBroom { size_t offset(const char* ptr) const; - void advance(const size_t offset); + void advance(size_t offset); void advance(); void reset(); @@ -89,7 +89,7 @@ namespace TrenchBroom { StatePtr m_state; TokenizerState m_snapshot; public: - SaveState(StatePtr state) : + explicit SaveState(StatePtr state) : m_state(state), m_snapshot(m_state->snapshot()) {} @@ -114,13 +114,13 @@ namespace TrenchBroom { m_state(std::make_shared(str.c_str(), str.c_str() + str.size(), escapableChars, escapeChar)) {} template - Tokenizer(Tokenizer& nestedTokenizer) : + explicit Tokenizer(Tokenizer& nestedTokenizer) : m_state(nestedTokenizer.m_state) {} Tokenizer(const Tokenizer& other) : m_state(other.m_state) {} - virtual ~Tokenizer() {} + virtual ~Tokenizer() = default; Token nextToken(const TokenType skipTokens = 0u) { auto token = emitToken(); @@ -152,8 +152,8 @@ namespace TrenchBroom { } Token token = peekToken(); - const auto* startPos = std::begin(token); - const auto* endPos = startPos; + const char* startPos = std::begin(token); + const char* endPos = startPos; do { token = nextToken(); endPos = std::end(token); @@ -166,8 +166,8 @@ namespace TrenchBroom { while (isWhitespace(curChar())) { advance(); } - const auto* startPos = curPos(); - const auto* endPos = (curChar() == '"' ? readQuotedString() : readUntil(delims)); + const char* startPos = curPos(); + const char* endPos = (curChar() == '"' ? readQuotedString() : readUntil(delims)); return String(startPos, static_cast(endPos - startPos)); } @@ -314,8 +314,9 @@ namespace TrenchBroom { private: void readDigits() { - while (!eof() && isDigit(curChar())) + while (!eof() && isDigit(curChar())) { advance(); + } } protected: const char* readUntil(const String& delims) { @@ -344,7 +345,7 @@ namespace TrenchBroom { advance(); } errorIfEof(); - const auto* end = curPos(); + const char* end = curPos(); advance(); return end; } @@ -393,7 +394,7 @@ namespace TrenchBroom { const char* discard(const String& str) { for (size_t i = 0; i < str.size(); ++i) { - const auto c = lookAhead(i); + const char c = lookAhead(i); if (c == 0 || c != str[i]) { return nullptr; } @@ -409,8 +410,8 @@ namespace TrenchBroom { } protected: bool isAnyOf(const char c, const String& allow) const { - for (size_t i = 0; i < allow.size(); i++) { - if (c == allow[i]) { + for (const auto& a : allow) { + if (c == a) { return true; } } diff --git a/common/src/IO/ZipFileSystem.cpp b/common/src/IO/ZipFileSystem.cpp index ab40fb1d8c..8d9fbda089 100644 --- a/common/src/IO/ZipFileSystem.cpp +++ b/common/src/IO/ZipFileSystem.cpp @@ -85,7 +85,8 @@ namespace TrenchBroom { } } - if (auto err = mz_zip_get_last_error(&m_archive); err != MZ_ZIP_NO_ERROR) { + const auto err = mz_zip_get_last_error(&m_archive); + if (err != MZ_ZIP_NO_ERROR) { throw FileSystemException(String("Error while reading compressed file: ") + mz_zip_get_error_string(err)); } } diff --git a/common/src/Model/Tag.cpp b/common/src/Model/Tag.cpp index db77a1a055..750d321d38 100644 --- a/common/src/Model/Tag.cpp +++ b/common/src/Model/Tag.cpp @@ -50,7 +50,7 @@ namespace TrenchBroom { m_attributes(std::move(attributes)) {} Tag::Tag(String name, std::vector attributes) : - Tag(0, name, attributes) {} + Tag(0, std::move(name), std::move(attributes)) {} Tag::~Tag() = default; @@ -207,7 +207,7 @@ namespace TrenchBroom { } SmartTag::SmartTag(String name, std::vector attributes, std::unique_ptr matcher) : - Tag(name, attributes), + Tag(std::move(name), std::move(attributes)), m_matcher(std::move(matcher)) {} SmartTag::SmartTag(const SmartTag& other) : diff --git a/common/src/Renderer/BrushRenderer.cpp b/common/src/Renderer/BrushRenderer.cpp index a41147eba5..2f87114ee7 100644 --- a/common/src/Renderer/BrushRenderer.cpp +++ b/common/src/Renderer/BrushRenderer.cpp @@ -139,7 +139,8 @@ namespace TrenchBroom { // update toAdd and toRemove using the input list for (const auto* brush : brushes) { - if (auto it = toRemove.find(brush); it != toRemove.end()) { + const auto it = toRemove.find(brush); + if (it != toRemove.end()) { toRemove.erase(it); } else { toAdd.insert(brush); @@ -176,7 +177,8 @@ namespace TrenchBroom { continue; } // if it's not in the invalid set, put it in - if (auto it = m_invalidBrushes.find(brush); it == m_invalidBrushes.end()) { + const auto it = m_invalidBrushes.find(brush); + if (it == m_invalidBrushes.end()) { removeBrushFromVbo(brush); m_invalidBrushes.insert(brush); } diff --git a/common/src/Renderer/BrushRendererArrays.cpp b/common/src/Renderer/BrushRendererArrays.cpp index 169f9f760f..37549f97a3 100644 --- a/common/src/Renderer/BrushRendererArrays.cpp +++ b/common/src/Renderer/BrushRendererArrays.cpp @@ -102,7 +102,8 @@ namespace TrenchBroom { } std::pair BrushIndexArray::getPointerToInsertElementsAt(const size_t elementCount) { - if (auto block = m_allocationTracker.allocate(elementCount); block != nullptr) { + auto block = m_allocationTracker.allocate(elementCount); + if (block != nullptr) { GLuint* dest = m_indexHolder.getPointerToWriteElementsTo(block->pos, elementCount); return {block, dest}; } @@ -114,8 +115,9 @@ namespace TrenchBroom { m_indexHolder.resize(newSize); // insert again - auto block = m_allocationTracker.allocate(elementCount); + block = m_allocationTracker.allocate(elementCount); assert(block != nullptr); + GLuint* dest = m_indexHolder.getPointerToWriteElementsTo(block->pos, elementCount); return {block, dest}; } @@ -148,7 +150,8 @@ namespace TrenchBroom { m_allocationTracker(0) {} std::pair BrushVertexArray::getPointerToInsertVerticesAt(const size_t vertexCount) { - if (auto block = m_allocationTracker.allocate(vertexCount); block != nullptr) { + auto block = m_allocationTracker.allocate(vertexCount); + if (block != nullptr) { Vertex* dest = m_vertexHolder.getPointerToWriteElementsTo(block->pos, vertexCount); return {block, dest}; } @@ -160,7 +163,7 @@ namespace TrenchBroom { m_vertexHolder.resize(newSize); // insert again - auto block = m_allocationTracker.allocate(vertexCount); + block = m_allocationTracker.allocate(vertexCount); assert(block != nullptr); Vertex* dest = m_vertexHolder.getPointerToWriteElementsTo(block->pos, vertexCount); diff --git a/common/src/Renderer/Renderable.cpp b/common/src/Renderer/Renderable.cpp index f124fa4e3f..2aa9fc0729 100644 --- a/common/src/Renderer/Renderable.cpp +++ b/common/src/Renderer/Renderable.cpp @@ -30,7 +30,5 @@ namespace TrenchBroom { void DirectRenderable::prepareVertices(Vbo& vertexVbo) { doPrepareVertices(vertexVbo); } - - IndexedRenderable::~IndexedRenderable() {} } } diff --git a/common/src/Renderer/Renderable.h b/common/src/Renderer/Renderable.h index e750a80157..6bf6a26619 100644 --- a/common/src/Renderer/Renderable.h +++ b/common/src/Renderer/Renderable.h @@ -44,7 +44,7 @@ namespace TrenchBroom { class DirectRenderable : public Renderable { public: DirectRenderable() = default; - ~DirectRenderable() = default; + ~DirectRenderable() override = default; void prepareVertices(Vbo& vertexVbo); private: @@ -56,7 +56,7 @@ namespace TrenchBroom { class IndexedRenderable : public Renderable { public: IndexedRenderable() = default; - ~IndexedRenderable() override; + ~IndexedRenderable() override = default; virtual void prepareVerticesAndIndices(Vbo& vertexVbo, Vbo& indexVbo) = 0; diff --git a/common/src/Renderer/ShaderProgram.cpp b/common/src/Renderer/ShaderProgram.cpp index 0c7a9f3494..611f0a1fff 100644 --- a/common/src/Renderer/ShaderProgram.cpp +++ b/common/src/Renderer/ShaderProgram.cpp @@ -30,11 +30,11 @@ namespace TrenchBroom { namespace Renderer { ShaderProgram::ShaderProgram(const String& name) : m_name(name), - m_programId(0), + m_programId(glCreateProgram()), m_needsLinking(true) { - glAssert(m_programId = glCreateProgram()); - if (m_programId == 0) + if (m_programId == 0) { throw RenderException("Cannot create shader program " + m_name); + } } ShaderProgram::~ShaderProgram() { diff --git a/common/src/Renderer/Vbo.cpp b/common/src/Renderer/Vbo.cpp index f856b9e3e6..e876111783 100644 --- a/common/src/Renderer/Vbo.cpp +++ b/common/src/Renderer/Vbo.cpp @@ -24,6 +24,7 @@ #include #include +#include namespace TrenchBroom { namespace Renderer { @@ -62,13 +63,14 @@ namespace TrenchBroom { } Vbo::~Vbo() { - if (active()) + if (active()) { deactivate(); + } free(); - auto* block = m_firstBlock; + VboBlock* block = m_firstBlock; while (block != nullptr) { - auto* next = block->next(); + VboBlock* next = block->next(); delete block; block = next; } @@ -92,14 +94,16 @@ namespace TrenchBroom { } assert(it != std::end(m_freeBlocks)); - auto* block = *it; + VboBlock* block = *it; + ensure(block != nullptr, "block is null"); removeFreeBlock(it); if (block->capacity() > capacity) { - auto* remainder = block->split(capacity); - if (m_lastBlock == block) + VboBlock* remainder = block->split(capacity); + if (m_lastBlock == block) { m_lastBlock = remainder; + } insertFreeBlock(remainder); } @@ -148,8 +152,8 @@ namespace TrenchBroom { assert(!block->isFree()); assert(checkBlockChain()); - auto* previous = block->previous(); - auto* next = block->next(); + VboBlock* previous = block->previous(); + VboBlock* next = block->next(); if (previous != nullptr && previous->isFree() && next != nullptr && next->isFree()) { @@ -157,23 +161,26 @@ namespace TrenchBroom { removeFreeBlock(next); previous->mergeWithSuccessor(); previous->mergeWithSuccessor(); - if (m_lastBlock == next) + if (m_lastBlock == next) { m_lastBlock = previous; + } delete block; delete next; insertFreeBlock(previous); } else if (previous != nullptr && previous->isFree()) { removeFreeBlock(previous); previous->mergeWithSuccessor(); - if (m_lastBlock == block) + if (m_lastBlock == block) { m_lastBlock = previous; + } delete block; insertFreeBlock(previous); } else if (next != nullptr && next->isFree()) { removeFreeBlock(next); block->mergeWithSuccessor(); - if (m_lastBlock == next) + if (m_lastBlock == next) { m_lastBlock = block; + } delete next; insertFreeBlock(block); } else { @@ -184,12 +191,14 @@ namespace TrenchBroom { void Vbo::increaseCapacityToAccomodate(const size_t capacity) { auto newMinCapacity = m_totalCapacity + capacity; - if (m_lastBlock->isFree()) + if (m_lastBlock->isFree()) { newMinCapacity -= m_lastBlock->capacity(); + } auto newCapacity = m_totalCapacity; - while (newCapacity < newMinCapacity) + while (newCapacity < newMinCapacity) { newCapacity = static_cast(static_cast(newCapacity) * GrowthFactor); + } increaseCapacity(newCapacity - m_totalCapacity); } @@ -209,7 +218,7 @@ namespace TrenchBroom { m_lastBlock->setCapacity(m_lastBlock->capacity() + delta); insertFreeBlock(m_lastBlock); } else { - auto* block = m_lastBlock->createSuccessor(delta); + auto block = m_lastBlock->createSuccessor(delta); m_lastBlock = block; insertFreeBlock(m_lastBlock); } @@ -219,10 +228,10 @@ namespace TrenchBroom { assert(checkBlockChain()); if (begin < end) { - auto* buffer = map(); + unsigned char* buffer = map(); - auto* temp = new unsigned char[end - begin]; - memcpy(temp, buffer + begin, end - begin); + auto temp = std::make_unique(end - begin); + memcpy(temp.get(), buffer + begin, end - begin); unmap(); deactivate(); @@ -230,8 +239,7 @@ namespace TrenchBroom { activate(); buffer = map(); - memcpy(buffer + begin, temp, end - begin); - delete [] temp; + memcpy(buffer + begin, temp.get(), end - begin); unmap(); } else { @@ -248,10 +256,11 @@ namespace TrenchBroom { void Vbo::insertFreeBlock(VboBlock* block) { auto it = std::lower_bound(std::begin(m_freeBlocks), std::end(m_freeBlocks), block, CompareVboBlocksByCapacity()); - if (it == std::end(m_freeBlocks)) + if (it == std::end(m_freeBlocks)) { m_freeBlocks.push_back(block); - else + } else { m_freeBlocks.insert(it, block); + } block->setFree(true); m_freeCapacity += block->capacity(); } @@ -261,8 +270,9 @@ namespace TrenchBroom { assert(it != std::end(m_freeBlocks)); if (*it != block) { const auto end = std::upper_bound(std::begin(m_freeBlocks), std::end(m_freeBlocks), block, CompareVboBlocksByCapacity()); - while (it != end && *it != block) + while (it != end && *it != block) { ++it; + } assert(it != end); } assert(*it == block); @@ -270,7 +280,7 @@ namespace TrenchBroom { } void Vbo::removeFreeBlock(const VboBlockList::iterator it) { - auto* block = *it; + VboBlock* block = *it; m_freeBlocks.erase(it); block->setFree(false); m_freeCapacity -= block->capacity(); @@ -306,7 +316,7 @@ namespace TrenchBroom { // fixes a crash on Mac OS X where a buffer could not be mapped after another windows was closed glAssert(glFinishObjectAPPLE(GL_BUFFER_OBJECT_APPLE, static_cast(m_vboId))); #endif - auto* buffer = reinterpret_cast(glMapBuffer(m_type, GL_WRITE_ONLY)); + unsigned char* buffer = reinterpret_cast(glMapBuffer(m_type, GL_WRITE_ONLY)); ensure(buffer != nullptr, "buffer is null"); m_state = State_FullyMapped; @@ -320,11 +330,11 @@ namespace TrenchBroom { } bool Vbo::checkBlockChain() const { - auto* block = m_firstBlock; + VboBlock* block = m_firstBlock; ensure(block != nullptr, "block is null"); auto count = 0u; - auto* next = block->next(); + VboBlock* next = block->next(); while (next != nullptr) { assert(next->previous() == block); assert(!block->isFree() || !next->isFree()); @@ -335,7 +345,7 @@ namespace TrenchBroom { assert(block == m_lastBlock); - auto* previous = block->previous(); + VboBlock* previous = block->previous(); while (previous != nullptr) { assert(previous->next() == block); block = previous; diff --git a/common/src/TemporarilySetAny.h b/common/src/TemporarilySetAny.h index 3aea96bd72..bca3e7abb1 100644 --- a/common/src/TemporarilySetAny.h +++ b/common/src/TemporarilySetAny.h @@ -29,7 +29,6 @@ namespace TrenchBroom { private: T& m_value; T m_oldValue; - T m_newValue; public: TemporarilySetAny(T& value, T newValue) : m_value(value), diff --git a/common/src/View/CameraTool2D.cpp b/common/src/View/CameraTool2D.cpp index ab8b351fcb..6c1caaa32b 100644 --- a/common/src/View/CameraTool2D.cpp +++ b/common/src/View/CameraTool2D.cpp @@ -38,6 +38,10 @@ namespace TrenchBroom { return this; } + const Tool* CameraTool2D::doGetTool() const { + return this; + } + void CameraTool2D::doMouseScroll(const InputState& inputState) { if (zoom(inputState)) { if (inputState.scrollY() != 0.0f) { diff --git a/common/src/View/CameraTool2D.h b/common/src/View/CameraTool2D.h index c6df8f080a..141e82a465 100644 --- a/common/src/View/CameraTool2D.h +++ b/common/src/View/CameraTool2D.h @@ -38,9 +38,10 @@ namespace TrenchBroom { Renderer::OrthographicCamera& m_camera; vm::vec2f m_lastMousePos; public: - CameraTool2D(Renderer::OrthographicCamera& camera); + explicit CameraTool2D(Renderer::OrthographicCamera& camera); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doMouseScroll(const InputState& inputState) override; bool doStartMouseDrag(const InputState& inputState) override; diff --git a/common/src/View/CameraTool3D.cpp b/common/src/View/CameraTool3D.cpp index 8a30c54981..aa4624b5a9 100644 --- a/common/src/View/CameraTool3D.cpp +++ b/common/src/View/CameraTool3D.cpp @@ -77,6 +77,10 @@ namespace TrenchBroom { return this; } + const Tool* CameraTool3D::doGetTool() const { + return this; + } + void CameraTool3D::doMouseScroll(const InputState& inputState) { const auto factor = pref(Preferences::CameraMouseWheelInvert) ? -1.0f : 1.0f; const auto zoom = inputState.modifierKeysPressed(ModifierKeys::MKShift); diff --git a/common/src/View/CameraTool3D.h b/common/src/View/CameraTool3D.h index 0fb93bec8a..f1c9de822a 100644 --- a/common/src/View/CameraTool3D.h +++ b/common/src/View/CameraTool3D.h @@ -44,6 +44,7 @@ namespace TrenchBroom { void fly(int dx, int dy, bool forward, bool backward, bool left, bool right, unsigned int time); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doMouseScroll(const InputState& inputState) override; bool doStartMouseDrag(const InputState& inputState) override; diff --git a/common/src/View/ClipToolController.cpp b/common/src/View/ClipToolController.cpp index eae36b826c..a06ede1f99 100644 --- a/common/src/View/ClipToolController.cpp +++ b/common/src/View/ClipToolController.cpp @@ -101,6 +101,10 @@ namespace TrenchBroom { return m_callback->tool(); } + const Tool* ClipToolController::AddClipPointPart::doGetTool() const { + return m_callback->tool(); + } + bool ClipToolController::AddClipPointPart::doMouseClick(const InputState& inputState) { if (!inputState.mouseButtonsPressed(MouseButtons::MBLeft) || !inputState.modifierKeysPressed(ModifierKeys::MKNone)) @@ -175,6 +179,10 @@ namespace TrenchBroom { return m_callback->tool(); } + const Tool* ClipToolController::MoveClipPointPart::doGetTool() const { + return m_callback->tool(); + } + RestrictedDragPolicy::DragInfo ClipToolController::MoveClipPointPart::doStartDrag(const InputState& inputState) { if (inputState.mouseButtons() != MouseButtons::MBLeft || inputState.modifierKeys() != ModifierKeys::MKNone) @@ -216,6 +224,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* ClipToolController::doGetTool() const { + return m_tool; + } + void ClipToolController::doPick(const InputState& inputState, Model::PickResult& pickResult) { m_tool->pick(inputState.pickRay(), inputState.camera(), pickResult); } diff --git a/common/src/View/ClipToolController.h b/common/src/View/ClipToolController.h index c94f65e635..1913ad4863 100644 --- a/common/src/View/ClipToolController.h +++ b/common/src/View/ClipToolController.h @@ -83,6 +83,8 @@ namespace TrenchBroom { AddClipPointPart(Callback* callback); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; + bool doMouseClick(const InputState& inputState) override; bool doMouseDoubleClick(const InputState& inputState) override; DragInfo doStartDrag(const InputState& inputState) override; @@ -98,6 +100,7 @@ namespace TrenchBroom { MoveClipPointPart(Callback* callback); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; DragInfo doStartDrag(const InputState& inputState) override; DragResult doDrag(const InputState& inputState, const vm::vec3& lastHandlePosition, const vm::vec3& nextHandlePosition) override; void doEndDrag(const InputState& inputState) override; @@ -111,6 +114,7 @@ namespace TrenchBroom { virtual ~ClipToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/CreateComplexBrushToolController3D.cpp b/common/src/View/CreateComplexBrushToolController3D.cpp index d001498a4c..b53f95e3f8 100644 --- a/common/src/View/CreateComplexBrushToolController3D.cpp +++ b/common/src/View/CreateComplexBrushToolController3D.cpp @@ -68,6 +68,7 @@ namespace TrenchBroom { Part(tool) {} private: Tool* doGetTool() override { return m_tool; } + const Tool* doGetTool() const override { return m_tool; } DragInfo doStartDrag(const InputState& inputState) override { if (inputState.modifierKeysDown(ModifierKeys::MKShift)) @@ -141,6 +142,7 @@ namespace TrenchBroom { Part(tool) {} private: Tool* doGetTool() override { return m_tool; } + const Tool* doGetTool() const override { return m_tool; } DragInfo doStartDrag(const InputState& inputState) override { if (!inputState.modifierKeysDown(ModifierKeys::MKShift)) @@ -207,6 +209,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* CreateComplexBrushToolController3D::doGetTool() const { + return m_tool; + } + bool CreateComplexBrushToolController3D::doMouseClick(const InputState& inputState) { if (!inputState.mouseButtonsDown(MouseButtons::MBLeft)) return false; diff --git a/common/src/View/CreateComplexBrushToolController3D.h b/common/src/View/CreateComplexBrushToolController3D.h index d42912826a..3ae8a2c1bd 100644 --- a/common/src/View/CreateComplexBrushToolController3D.h +++ b/common/src/View/CreateComplexBrushToolController3D.h @@ -42,6 +42,7 @@ namespace TrenchBroom { explicit CreateComplexBrushToolController3D(CreateComplexBrushTool* tool); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; bool doMouseClick(const InputState& inputState) override; bool doMouseDoubleClick(const InputState& inputState) override; diff --git a/common/src/View/CreateEntityToolController.cpp b/common/src/View/CreateEntityToolController.cpp index 223ac21f10..9a30d55ff4 100644 --- a/common/src/View/CreateEntityToolController.cpp +++ b/common/src/View/CreateEntityToolController.cpp @@ -37,6 +37,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* CreateEntityToolController::doGetTool() const { + return m_tool; + } + bool CreateEntityToolController::doDragEnter(const InputState& inputState, const String& payload) { const StringList parts = StringUtils::split(payload, ':'); if (parts.size() != 2) diff --git a/common/src/View/CreateEntityToolController.h b/common/src/View/CreateEntityToolController.h index 92a8e44635..0ec34c5729 100644 --- a/common/src/View/CreateEntityToolController.h +++ b/common/src/View/CreateEntityToolController.h @@ -37,6 +37,7 @@ namespace TrenchBroom { virtual ~CreateEntityToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; bool doDragEnter(const InputState& inputState, const String& payload) override; bool doDragMove(const InputState& inputState) override; diff --git a/common/src/View/CreateSimpleBrushToolController2D.cpp b/common/src/View/CreateSimpleBrushToolController2D.cpp index 66554555ce..7eb07f7ff0 100644 --- a/common/src/View/CreateSimpleBrushToolController2D.cpp +++ b/common/src/View/CreateSimpleBrushToolController2D.cpp @@ -43,6 +43,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* CreateSimpleBrushToolController2D::doGetTool() const { + return m_tool; + } + RestrictedDragPolicy::DragInfo CreateSimpleBrushToolController2D::doStartDrag(const InputState& inputState) { if (!inputState.mouseButtonsPressed(MouseButtons::MBLeft)) { return DragInfo(); diff --git a/common/src/View/CreateSimpleBrushToolController2D.h b/common/src/View/CreateSimpleBrushToolController2D.h index 4761ad4e2d..50c33c1511 100644 --- a/common/src/View/CreateSimpleBrushToolController2D.h +++ b/common/src/View/CreateSimpleBrushToolController2D.h @@ -38,6 +38,7 @@ namespace TrenchBroom { CreateSimpleBrushToolController2D(CreateSimpleBrushTool* tool, MapDocumentWPtr document); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; DragInfo doStartDrag(const InputState& inputState) override; DragResult doDrag(const InputState& inputState, const vm::vec3& lastHandlePosition, const vm::vec3& nextHandlePosition) override; diff --git a/common/src/View/CreateSimpleBrushToolController3D.cpp b/common/src/View/CreateSimpleBrushToolController3D.cpp index effcab7d5c..4568c89636 100644 --- a/common/src/View/CreateSimpleBrushToolController3D.cpp +++ b/common/src/View/CreateSimpleBrushToolController3D.cpp @@ -55,6 +55,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* CreateSimpleBrushToolController3D::doGetTool() const { + return m_tool; + } + void CreateSimpleBrushToolController3D::doModifierKeyChange(const InputState& inputState) { if (thisToolDragging()) { if (inputState.modifierKeys() == ModifierKeys::MKAlt) { diff --git a/common/src/View/CreateSimpleBrushToolController3D.h b/common/src/View/CreateSimpleBrushToolController3D.h index 0f6d255f51..fd58447097 100644 --- a/common/src/View/CreateSimpleBrushToolController3D.h +++ b/common/src/View/CreateSimpleBrushToolController3D.h @@ -44,6 +44,7 @@ namespace TrenchBroom { CreateSimpleBrushToolController3D(CreateSimpleBrushTool* tool, MapDocumentWPtr document); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doModifierKeyChange(const InputState& inputState) override; diff --git a/common/src/View/KeyboardShortcut.cpp b/common/src/View/KeyboardShortcut.cpp index c5ffc9861b..d07e323495 100644 --- a/common/src/View/KeyboardShortcut.cpp +++ b/common/src/View/KeyboardShortcut.cpp @@ -34,28 +34,22 @@ namespace TrenchBroom { namespace View { bool KeyboardShortcut::MacModifierOrder::operator()(const int lhs, const int rhs) const { - if (lhs == WXK_NONE) - return lhs != WXK_NONE; if (lhs == WXK_ALT) return rhs != WXK_ALT; if (lhs == WXK_SHIFT) return rhs != WXK_ALT && rhs != WXK_SHIFT; if (lhs == WXK_CONTROL) return rhs == WXK_NONE; - assert(false); return false; } bool KeyboardShortcut::WinModifierOrder::operator()(const int lhs, const int rhs) const { - if (lhs == WXK_NONE) - return lhs != WXK_NONE; if (lhs == WXK_CONTROL) return rhs != WXK_CONTROL; if (lhs == WXK_ALT) return rhs != WXK_CONTROL && rhs != WXK_ALT; if (lhs == WXK_SHIFT) return rhs == WXK_NONE; - assert(false); return false; } diff --git a/common/src/View/KeyboardShortcutGridTable.cpp b/common/src/View/KeyboardShortcutGridTable.cpp index 3c593c2d0a..e24f3f93a9 100644 --- a/common/src/View/KeyboardShortcutGridTable.cpp +++ b/common/src/View/KeyboardShortcutGridTable.cpp @@ -47,8 +47,10 @@ namespace TrenchBroom { } wxString KeyboardShortcutGridTable::GetValue(const int row, const int col) { - assert(row >= 0 && row < GetNumberRows()); - assert(col >= 0 && col < GetNumberCols()); + if (row < 0 || row >= GetNumberRows() || + col < 0 || row >= GetNumberCols()) { + return ""; + } const size_t rowIndex = static_cast(row); @@ -68,8 +70,9 @@ namespace TrenchBroom { } void KeyboardShortcutGridTable::SetValue(const int row, const int col, const wxString& value) { - assert(row >= 0 && row < GetNumberRows()); - assert(col == 0); + if (row < 0 || row >= GetNumberRows() || col != 0) { + return; + } int key, modifier1, modifier2, modifier3; const bool success = KeyboardShortcut::parseShortcut(value, key, modifier1, modifier2, modifier3); @@ -106,7 +109,9 @@ namespace TrenchBroom { } wxString KeyboardShortcutGridTable::GetColLabelValue(const int col) { - assert(col >= 0 && col < GetNumberCols()); + if (col < 0 || col >= GetNumberCols()) { + return ""; + } switch (col) { case 0: return "Shortcut"; diff --git a/common/src/View/MoveObjectsToolController.cpp b/common/src/View/MoveObjectsToolController.cpp index 58df208bfc..55d8458f61 100644 --- a/common/src/View/MoveObjectsToolController.cpp +++ b/common/src/View/MoveObjectsToolController.cpp @@ -44,6 +44,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* MoveObjectsToolController::doGetTool() const { + return m_tool; + } + MoveObjectsToolController::MoveInfo MoveObjectsToolController::doStartMove(const InputState& inputState) { if (!inputState.modifierKeysPressed(ModifierKeys::MKNone) && !inputState.modifierKeysPressed(ModifierKeys::MKAlt) && diff --git a/common/src/View/MoveObjectsToolController.h b/common/src/View/MoveObjectsToolController.h index c6f4094934..48a00cd4f7 100644 --- a/common/src/View/MoveObjectsToolController.h +++ b/common/src/View/MoveObjectsToolController.h @@ -34,6 +34,7 @@ namespace TrenchBroom { virtual ~MoveObjectsToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; MoveInfo doStartMove(const InputState& inputState) override; DragResult doMove(const InputState& inputState, const vm::vec3& lastHandlePosition, const vm::vec3& nextHandlePosition) override; diff --git a/common/src/View/ResizeBrushesToolController.cpp b/common/src/View/ResizeBrushesToolController.cpp index c5856d8302..aca440e478 100644 --- a/common/src/View/ResizeBrushesToolController.cpp +++ b/common/src/View/ResizeBrushesToolController.cpp @@ -48,6 +48,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* ResizeBrushesToolController::doGetTool() const { + return m_tool; + } + void ResizeBrushesToolController::doPick(const InputState& inputState, Model::PickResult& pickResult) { if (handleInput(inputState)) { const Model::Hit hit = doPick(inputState.pickRay(), pickResult); diff --git a/common/src/View/ResizeBrushesToolController.h b/common/src/View/ResizeBrushesToolController.h index b7274cbcff..917434b507 100644 --- a/common/src/View/ResizeBrushesToolController.h +++ b/common/src/View/ResizeBrushesToolController.h @@ -53,6 +53,7 @@ namespace TrenchBroom { virtual ~ResizeBrushesToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/RotateObjectsToolController.cpp b/common/src/View/RotateObjectsToolController.cpp index 4746d68b68..45f404eda1 100644 --- a/common/src/View/RotateObjectsToolController.cpp +++ b/common/src/View/RotateObjectsToolController.cpp @@ -62,6 +62,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* doGetTool() const override { + return m_tool; + } + bool doMouseClick(const InputState& inputState) override { if (!inputState.mouseButtonsPressed(MouseButtons::MBLeft)) return false; @@ -209,6 +213,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* doGetTool() const override { + return m_tool; + } + MoveInfo doStartMove(const InputState& inputState) override { if (!inputState.mouseButtonsPressed(MouseButtons::MBLeft) || !inputState.checkModifierKeys(ModifierKeyPressed::MK_No, ModifierKeyPressed::MK_DontCare, ModifierKeyPressed::MK_No)) @@ -262,6 +270,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* RotateObjectsToolController::doGetTool() const { + return m_tool; + } + void RotateObjectsToolController::doPick(const InputState& inputState, Model::PickResult& pickResult) { const Model::Hit hit = doPick(inputState); if (hit.isMatch()) diff --git a/common/src/View/RotateObjectsToolController.h b/common/src/View/RotateObjectsToolController.h index e3f9093eeb..e50ff05e7f 100644 --- a/common/src/View/RotateObjectsToolController.h +++ b/common/src/View/RotateObjectsToolController.h @@ -47,6 +47,7 @@ namespace TrenchBroom { virtual ~RotateObjectsToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/ScaleObjectsTool.cpp b/common/src/View/ScaleObjectsTool.cpp index 3ce3130955..ddf84e7f86 100644 --- a/common/src/View/ScaleObjectsTool.cpp +++ b/common/src/View/ScaleObjectsTool.cpp @@ -323,8 +323,8 @@ namespace TrenchBroom { if (newCorner[i] == anchor[i]) { return vm::bbox3(); } - const auto oldPositive = oldCorner[i] > anchor[i]; - const auto newPositive = newCorner[i] > anchor[i]; + const bool oldPositive = oldCorner[i] > anchor[i]; + const bool newPositive = newCorner[i] > anchor[i]; if (oldPositive != newPositive) { return vm::bbox3(); } @@ -342,7 +342,7 @@ namespace TrenchBroom { vm::bbox3 moveBBoxEdge(const vm::bbox3& in, const BBoxEdge& edge, const vm::vec3& delta, - const ProportionalAxes proportional, + const ProportionalAxes& proportional, const AnchorPos anchorType) { const auto opposite = oppositeEdge(edge); diff --git a/common/src/View/ScaleObjectsTool.h b/common/src/View/ScaleObjectsTool.h index 8270434147..860e53d1ff 100644 --- a/common/src/View/ScaleObjectsTool.h +++ b/common/src/View/ScaleObjectsTool.h @@ -161,7 +161,7 @@ namespace TrenchBroom { vm::bbox3 moveBBoxEdge(const vm::bbox3& in, const BBoxEdge& edge, const vm::vec3& delta, - ProportionalAxes proportional, + const ProportionalAxes& proportional, AnchorPos anchor); /** diff --git a/common/src/View/ScaleObjectsToolController.cpp b/common/src/View/ScaleObjectsToolController.cpp index a74de54dd3..59f0786780 100644 --- a/common/src/View/ScaleObjectsToolController.cpp +++ b/common/src/View/ScaleObjectsToolController.cpp @@ -39,8 +39,7 @@ namespace TrenchBroom { namespace View { ScaleObjectsToolController::ScaleObjectsToolController(ScaleObjectsTool* tool, MapDocumentWPtr document) : m_tool(tool), - m_document(document) - { + m_document(document) { ensure(m_tool != nullptr, "tool is null"); } @@ -50,6 +49,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* ScaleObjectsToolController::doGetTool() const { + return m_tool; + } + void ScaleObjectsToolController::doPick(const InputState& inputState, Model::PickResult& pickResult) { if (handleInput(inputState)) { doPick(inputState.pickRay(), inputState.camera(), pickResult); diff --git a/common/src/View/ScaleObjectsToolController.h b/common/src/View/ScaleObjectsToolController.h index 417958999d..e3c32dff8d 100644 --- a/common/src/View/ScaleObjectsToolController.h +++ b/common/src/View/ScaleObjectsToolController.h @@ -51,6 +51,7 @@ namespace TrenchBroom { ~ScaleObjectsToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; virtual void doPick(const vm::ray3 &pickRay, const Renderer::Camera &camera, Model::PickResult &pickResult) = 0; diff --git a/common/src/View/SelectionTool.cpp b/common/src/View/SelectionTool.cpp index 890ef6629d..c0f74a369b 100644 --- a/common/src/View/SelectionTool.cpp +++ b/common/src/View/SelectionTool.cpp @@ -49,6 +49,10 @@ namespace TrenchBroom { return this; } + const Tool* SelectionTool::doGetTool() const { + return this; + } + bool SelectionTool::doMouseClick(const InputState& inputState) { if (!handleClick(inputState)) { return false; diff --git a/common/src/View/SelectionTool.h b/common/src/View/SelectionTool.h index 06ba3adaff..40495dee8a 100644 --- a/common/src/View/SelectionTool.h +++ b/common/src/View/SelectionTool.h @@ -41,6 +41,7 @@ namespace TrenchBroom { SelectionTool(MapDocumentWPtr document); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; bool doMouseClick(const InputState& inputState) override; bool doMouseDoubleClick(const InputState& inputState) override; diff --git a/common/src/View/SetBrushFaceAttributesTool.cpp b/common/src/View/SetBrushFaceAttributesTool.cpp index e6efecf32a..7cd87bd045 100644 --- a/common/src/View/SetBrushFaceAttributesTool.cpp +++ b/common/src/View/SetBrushFaceAttributesTool.cpp @@ -36,6 +36,10 @@ namespace TrenchBroom { return this; } + const Tool* SetBrushFaceAttributesTool::doGetTool() const { + return this; + } + bool SetBrushFaceAttributesTool::doMouseClick(const InputState& inputState) { return performCopy(inputState, false); } diff --git a/common/src/View/SetBrushFaceAttributesTool.h b/common/src/View/SetBrushFaceAttributesTool.h index 2befd404fc..88f3b49b87 100644 --- a/common/src/View/SetBrushFaceAttributesTool.h +++ b/common/src/View/SetBrushFaceAttributesTool.h @@ -33,6 +33,7 @@ namespace TrenchBroom { SetBrushFaceAttributesTool(MapDocumentWPtr document); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; bool doMouseClick(const InputState& inputState) override; bool doMouseDoubleClick(const InputState& inputState) override; diff --git a/common/src/View/ShearObjectsToolController.cpp b/common/src/View/ShearObjectsToolController.cpp index 854828a3ac..e118032c28 100644 --- a/common/src/View/ShearObjectsToolController.cpp +++ b/common/src/View/ShearObjectsToolController.cpp @@ -49,6 +49,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* ShearObjectsToolController::doGetTool() const { + return m_tool; + } + void ShearObjectsToolController::doPick(const InputState& inputState, Model::PickResult& pickResult) { if (m_tool->applies()) { // forward to either ShearObjectsTool::pick2D or ShearObjectsTool::pick3D diff --git a/common/src/View/ShearObjectsToolController.h b/common/src/View/ShearObjectsToolController.h index c3bc8d1466..1e930ec69c 100644 --- a/common/src/View/ShearObjectsToolController.h +++ b/common/src/View/ShearObjectsToolController.h @@ -51,6 +51,7 @@ namespace TrenchBroom { ~ShearObjectsToolController() override; private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; virtual void doPick(const vm::ray3 &pickRay, const Renderer::Camera &camera, Model::PickResult &pickResult) = 0; diff --git a/common/src/View/ToolController.cpp b/common/src/View/ToolController.cpp index 7d94e75fe0..4adcd74c29 100644 --- a/common/src/View/ToolController.cpp +++ b/common/src/View/ToolController.cpp @@ -466,7 +466,8 @@ namespace TrenchBroom { ToolController::~ToolController() = default; Tool* ToolController::tool() { return doGetTool(); } - bool ToolController::toolActive() { return tool()->active(); } + const Tool* ToolController::tool() const { return doGetTool(); } + bool ToolController::toolActive() const { return tool()->active(); } void ToolController::refreshViews() { tool()->refreshViews(); } ToolControllerGroup::ToolControllerGroup() : diff --git a/common/src/View/ToolController.h b/common/src/View/ToolController.h index 400710b6e0..5eb74b69e7 100644 --- a/common/src/View/ToolController.h +++ b/common/src/View/ToolController.h @@ -364,7 +364,8 @@ namespace TrenchBroom { virtual ~ToolController(); Tool* tool(); - bool toolActive(); + const Tool* tool() const; + bool toolActive() const; virtual void pick(const InputState& inputState, Model::PickResult& pickResult) = 0; @@ -397,6 +398,7 @@ namespace TrenchBroom { void refreshViews(); private: virtual Tool* doGetTool() = 0; + virtual const Tool* doGetTool() const = 0; }; template diff --git a/common/src/View/UVCameraTool.cpp b/common/src/View/UVCameraTool.cpp index f5e23bfa88..c90fba48f8 100644 --- a/common/src/View/UVCameraTool.cpp +++ b/common/src/View/UVCameraTool.cpp @@ -36,6 +36,10 @@ namespace TrenchBroom { return this; } + const Tool* UVCameraTool::doGetTool() const { + return this; + } + void UVCameraTool::doMouseScroll(const InputState& inputState) { const auto oldWorldPos = m_camera.unproject(float(inputState.mouseX()), float(inputState.mouseY()), 0.0f); diff --git a/common/src/View/UVCameraTool.h b/common/src/View/UVCameraTool.h index 558793f049..cdc1fd1678 100644 --- a/common/src/View/UVCameraTool.h +++ b/common/src/View/UVCameraTool.h @@ -37,6 +37,7 @@ namespace TrenchBroom { UVCameraTool(Renderer::OrthographicCamera& camera); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doMouseScroll(const InputState& inputState) override; bool doStartMouseDrag(const InputState& inputState) override; diff --git a/common/src/View/UVOffsetTool.cpp b/common/src/View/UVOffsetTool.cpp index db133da507..54034e0f74 100644 --- a/common/src/View/UVOffsetTool.cpp +++ b/common/src/View/UVOffsetTool.cpp @@ -45,6 +45,10 @@ namespace TrenchBroom { return this; } + const Tool* UVOffsetTool::doGetTool() const { + return this; + } + bool UVOffsetTool::doStartMouseDrag(const InputState& inputState) { assert(m_helper.valid()); diff --git a/common/src/View/UVOffsetTool.h b/common/src/View/UVOffsetTool.h index 5299ad5769..5d1bb89575 100644 --- a/common/src/View/UVOffsetTool.h +++ b/common/src/View/UVOffsetTool.h @@ -38,6 +38,7 @@ namespace TrenchBroom { UVOffsetTool(MapDocumentWPtr document, const UVViewHelper& helper); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; bool doStartMouseDrag(const InputState& inputState) override; bool doMouseDrag(const InputState& inputState) override; diff --git a/common/src/View/UVOriginTool.cpp b/common/src/View/UVOriginTool.cpp index 57aa9b8dce..5ee381a392 100644 --- a/common/src/View/UVOriginTool.cpp +++ b/common/src/View/UVOriginTool.cpp @@ -61,6 +61,10 @@ namespace TrenchBroom { return this; } + const Tool* UVOriginTool::doGetTool() const { + return this; + } + void UVOriginTool::doPick(const InputState& inputState, Model::PickResult& pickResult) { if (m_helper.valid()) { vm::line3 xHandle, yHandle; diff --git a/common/src/View/UVOriginTool.h b/common/src/View/UVOriginTool.h index f526498687..d972afbce2 100644 --- a/common/src/View/UVOriginTool.h +++ b/common/src/View/UVOriginTool.h @@ -57,6 +57,7 @@ namespace TrenchBroom { UVOriginTool(UVViewHelper& helper); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/UVRotateTool.cpp b/common/src/View/UVRotateTool.cpp index ba33e64fec..998b658cd5 100644 --- a/common/src/View/UVRotateTool.cpp +++ b/common/src/View/UVRotateTool.cpp @@ -63,6 +63,10 @@ namespace TrenchBroom { return this; } + const Tool* UVRotateTool::doGetTool() const { + return this; + } + void UVRotateTool::doPick(const InputState& inputState, Model::PickResult& pickResult) { if (!m_helper.valid()) return; diff --git a/common/src/View/UVRotateTool.h b/common/src/View/UVRotateTool.h index 40da9db090..180d8c2cd8 100644 --- a/common/src/View/UVRotateTool.h +++ b/common/src/View/UVRotateTool.h @@ -54,6 +54,7 @@ namespace TrenchBroom { UVRotateTool(MapDocumentWPtr document, UVViewHelper& helper); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/UVScaleTool.cpp b/common/src/View/UVScaleTool.cpp index f518c430a5..11b48c5c14 100644 --- a/common/src/View/UVScaleTool.cpp +++ b/common/src/View/UVScaleTool.cpp @@ -57,6 +57,10 @@ namespace TrenchBroom { return this; } + const Tool* UVScaleTool::doGetTool() const { + return this; + } + void UVScaleTool::doPick(const InputState& inputState, Model::PickResult& pickResult) { static const Model::Hit::HitType HitTypes[] = { XHandleHit, YHandleHit }; if (m_helper.valid()) { diff --git a/common/src/View/UVScaleTool.h b/common/src/View/UVScaleTool.h index 6730cdb11e..e4677b91a2 100644 --- a/common/src/View/UVScaleTool.h +++ b/common/src/View/UVScaleTool.h @@ -60,6 +60,7 @@ namespace TrenchBroom { UVScaleTool(MapDocumentWPtr document, UVViewHelper& helper); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/UVShearTool.cpp b/common/src/View/UVShearTool.cpp index 642d3f8fdf..6925f4e35b 100644 --- a/common/src/View/UVShearTool.cpp +++ b/common/src/View/UVShearTool.cpp @@ -44,6 +44,10 @@ namespace TrenchBroom { return this; } + const Tool* UVShearTool::doGetTool() const { + return this; + } + void UVShearTool::doPick(const InputState& inputState, Model::PickResult& pickResult) { static const Model::Hit::HitType HitTypes[] = { XHandleHit, YHandleHit }; if (m_helper.valid()) diff --git a/common/src/View/UVShearTool.h b/common/src/View/UVShearTool.h index 93f86a4249..e6b7b45f77 100644 --- a/common/src/View/UVShearTool.h +++ b/common/src/View/UVShearTool.h @@ -46,6 +46,7 @@ namespace TrenchBroom { UVShearTool(MapDocumentWPtr document, UVViewHelper& helper); private: Tool* doGetTool() override; + const Tool* doGetTool() const override; void doPick(const InputState& inputState, Model::PickResult& pickResult) override; diff --git a/common/src/View/VertexToolControllerBase.h b/common/src/View/VertexToolControllerBase.h index 85d970abc2..c83fd3ab88 100644 --- a/common/src/View/VertexToolControllerBase.h +++ b/common/src/View/VertexToolControllerBase.h @@ -112,6 +112,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* doGetTool() const override { + return m_tool; + } + void doPick(const InputState& inputState, Model::PickResult& pickResult) override { m_tool->pick(inputState.pickRay(), inputState.camera(), pickResult); } @@ -246,6 +250,10 @@ namespace TrenchBroom { return m_tool; } + const Tool* doGetTool() const override { + return m_tool; + } + bool doCancel() override { return m_tool->deselectAll(); } @@ -321,6 +329,10 @@ namespace TrenchBroom { Tool* doGetTool() override { return m_tool; } + + const Tool* doGetTool() const override { + return m_tool; + } }; } } diff --git a/cppcheck.suppr b/cppcheck.suppr new file mode 100644 index 0000000000..b923ec8bea --- /dev/null +++ b/cppcheck.suppr @@ -0,0 +1,10 @@ +// suppress vm::vec(...) constructor warnings for the constructors directly accepting values for the vector components +// these constructors are disabled for invalid use cases (e.g. vm::vec3f(1, 2, 3, 4)) using SFINAE, but cppcheck considers them anyway +arrayIndexOutOfBounds:*/vec.h:262 +arrayIndexOutOfBounds:*/vec.h:289 +arrayIndexOutOfBounds:*/vec.h:290 + +// supress vm::vec::x() vm::vec::y() vm::vec::z() vm::vec::w() errors accessing nonexisting values +// these functions are disabled for invalid use cases (e.g. vm::vec2f().z()) using SFINAE, but cppcheck considers them anyway +arrayIndexOutOfBounds:*/vec.h:394 +arrayIndexOutOfBounds:*/vec.h:405 \ No newline at end of file diff --git a/test/src/Renderer/VertexTest.cpp b/test/src/Renderer/VertexTest.cpp index 4615f79247..fde9ca2bf0 100644 --- a/test/src/Renderer/VertexTest.cpp +++ b/test/src/Renderer/VertexTest.cpp @@ -39,7 +39,7 @@ namespace TrenchBroom { using Vertex = GLVertexTypes::P3T2C4::Vertex; const auto pos = vm::vec3f(1.0f, 2.0f, 3.0f); - const auto uv = vm::vec2f(4.0f, 5.0f, 6.0f); + const auto uv = vm::vec2f(4.0f, 5.0f); const auto color = vm::vec4f(7.0f, 8.0f, 9.0f, 10.0f); const auto expected = TestVertex{ pos, uv, color }; @@ -58,7 +58,7 @@ namespace TrenchBroom { for (size_t i = 0; i < 3; ++i) { const auto f = static_cast(i); const auto pos = f * vm::vec3f(1.0f, 2.0f, 3.0f); - const auto uv = f * vm::vec2f(4.0f, 5.0f, 6.0f); + const auto uv = f * vm::vec2f(4.0f, 5.0f); const auto color = f * vm::vec4f(7.0f, 8.0f, 9.0f, 10.0f); expected.emplace_back(TestVertex{ pos, uv, color }); diff --git a/test/src/View/MoveToolControllerTest.cpp b/test/src/View/MoveToolControllerTest.cpp index 24b735a99d..7d43a64369 100644 --- a/test/src/View/MoveToolControllerTest.cpp +++ b/test/src/View/MoveToolControllerTest.cpp @@ -56,6 +56,7 @@ namespace TrenchBroom { mockDoCancelMove(); } Tool* doGetTool() override { return &m_tool; } + const Tool* doGetTool() const override { return &m_tool; } bool doCancel() override { return false; } public: MOCK_METHOD1(mockDoStartMove, MoveInfo(const InputState&)); diff --git a/test/src/vec_test.cpp b/test/src/vec_test.cpp index 7847600cd0..1a75d0c2f3 100644 --- a/test/src/vec_test.cpp +++ b/test/src/vec_test.cpp @@ -128,10 +128,6 @@ namespace vm { ASSERT_EQ(vec3f(1.0f, 2.0f, 0.0f), vec3f(1.0f, 2.0f)); } - TEST(VecTest, constructVec3fFrom4Floats) { - ASSERT_EQ(vec3f(1.0f, 2.0f, 3.0f), vec3f(1.0f, 2.0f, 3.0f, 4.0f)); - } - TEST(VecTest, constructVec4fFrom3Floats) { ASSERT_EQ(vec4f(1.0f, 2.0f, 3.0f, 0.0f), vec4f(1.0f, 2.0f, 3.0f)); } diff --git a/travis-linux.sh b/travis-linux.sh index fbb26e4001..68757840c3 100755 --- a/travis-linux.sh +++ b/travis-linux.sh @@ -4,7 +4,7 @@ set -o verbose sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test sudo apt-get -qq update -sudo apt-get -y install libgtk2.0-dev freeglut3 freeglut3-dev libglew-dev mesa-common-dev build-essential libglm-dev libxxf86vm-dev libfreeimage-dev pandoc cmake p7zip-full ninja-build xvfb rpm +sudo apt-get -y install libgtk2.0-dev freeglut3 freeglut3-dev libglew-dev mesa-common-dev build-essential libglm-dev libxxf86vm-dev libfreeimage-dev pandoc cmake p7zip-full ninja-build xvfb rpm cppcheck if [[ $TB_GCC8 == "true" ]] ; then export CC=gcc-8 @@ -45,6 +45,8 @@ fi mkdir build cd build cmake .. -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS=-Werror -DwxWidgets_PREFIX=$WX_CACHE_FULLPATH || exit 1 +# disable cppcheck on linux because the binary is outdated and detects to many false positives +# cmake --build . --target cppcheck || exit 1 cmake --build . --config Release || exit 1 cpack || exit 1 diff --git a/travis-macos.sh b/travis-macos.sh index a6a91d8c5a..657d80b204 100755 --- a/travis-macos.sh +++ b/travis-macos.sh @@ -3,7 +3,7 @@ set -o verbose brew update -brew install cmake p7zip pandoc +brew install cmake p7zip pandoc cppcheck # Patch and build wxWidgets @@ -45,6 +45,7 @@ echo "TB_ENABLE_ASAN: $TB_ENABLE_ASAN_VALUE" mkdir build cd build cmake .. -GXcode -DCMAKE_BUILD_TYPE="$BUILD_TYPE_VALUE" -DCMAKE_CXX_FLAGS="-Werror" -DTB_ENABLE_ASAN="$TB_ENABLE_ASAN_VALUE" -DwxWidgets_PREFIX=$WX_CACHE_FULLPATH || exit 1 +cmake --build . --target cppcheck || exit 1 cmake --build . --config "$BUILD_TYPE_VALUE" || exit 1 cpack -C $BUILD_TYPE_VALUE || exit 1 diff --git a/vecmath/include/vecmath/vec.h b/vecmath/include/vecmath/vec.h index ef97648c9a..bcb8ff0499 100644 --- a/vecmath/include/vecmath/vec.h +++ b/vecmath/include/vecmath/vec.h @@ -26,6 +26,7 @@ along with TrenchBroom. If not, see . #include #include #include +#include namespace vm { template @@ -105,7 +106,7 @@ namespace vm { /** * Returns whether parse() can parse S components from the given string - * + * * @param str the string to parse * @return whether S components can be parsed */ @@ -225,17 +226,15 @@ namespace vm { * * @tparam U1 the type of the first given value * @tparam U2 the type of the second given value + * @tparam SS the number of components of this vector (only used for std::enable_if) * @param i_x the value of the first component * @param i_y the value of the second component */ - template - vec(U1 i_x, U2 i_y) { - if (S > 0) { - v[0] = static_cast(i_x); - if (S > 1) { - v[1] = static_cast(i_y); - } - } + template + vec(U1 i_x, U2 i_y, typename std::enable_if= 2>::type* = 0) { + static_assert(S >= 2, "vector must have at least two components"); + v[0] = static_cast(i_x); + v[1] = static_cast(i_y); for (size_t i = 2; i < S; ++i) { v[i] = static_cast(0.0); } @@ -250,21 +249,17 @@ namespace vm { * @tparam U1 the type of the first given value * @tparam U2 the type of the second given value * @tparam U3 the type of the third given value + * @tparam SS the number of components of this vector (only used for std::enable_if) * @param i_x the value of the first component * @param i_y the value of the second component * @param i_z the value of the third component */ - template - vec(U1 i_x, U2 i_y, U3 i_z) { - if (S > 0) { - v[0] = static_cast(i_x); - if (S > 1) { - v[1] = static_cast(i_y); - if (S > 2) { - v[2] = static_cast(i_z); - } - } - } + template + vec(U1 i_x, U2 i_y, U3 i_z, typename std::enable_if= 3>::type* = 0) { + static_assert(S >= 3, "vector must have at least three components"); + v[0] = static_cast(i_x); + v[1] = static_cast(i_y); + v[2] = static_cast(i_z); for (size_t i = 3; i < S; ++i) { v[i] = static_cast(0.0); } @@ -280,25 +275,19 @@ namespace vm { * @tparam U2 the type of the second given value * @tparam U3 the type of the third given value * @tparam U4 the type of the fourth given value + * @tparam SS the number of components of this vector (only used for std::enable_if) * @param i_x the value of the first component * @param i_y the value of the second component * @param i_z the value of the third component * @param i_w the value of the fourth component */ - template - vec(U1 i_x, U2 i_y, U3 i_z, U4 i_w) { - if (S > 0) { - v[0] = static_cast(i_x); - if (S > 1) { - v[1] = static_cast(i_y); - if (S > 2) { - v[2] = static_cast(i_z); - if (S > 3) { - v[3] = static_cast(i_w); - } - } - } - } + template + vec(U1 i_x, U2 i_y, U3 i_z, U4 i_w, typename std::enable_if= 4>::type* = 0) { + static_assert(S >= 4, "vector must have at least four components"); + v[0] = static_cast(i_x); + v[1] = static_cast(i_y); + v[2] = static_cast(i_z); + v[3] = static_cast(i_w); for (size_t i = 4; i < S; ++i) { v[i] = static_cast(0.0); } @@ -377,7 +366,8 @@ namespace vm { * * @return the value of the first component */ - T x() const { + template + T x(typename std::enable_if= 1>::type* = 0) const { static_assert(S > 0); return v[0]; } @@ -387,7 +377,8 @@ namespace vm { * * @return the value of the component component */ - T y() const { + template + T y(typename std::enable_if= 2>::type* = 0) const { static_assert(S > 1); return v[1]; } @@ -397,7 +388,8 @@ namespace vm { * * @return the value of the third component */ - T z() const { + template + T z (typename std::enable_if= 3>::type* = 0) const { static_assert(S > 2); return v[2]; } @@ -407,7 +399,8 @@ namespace vm { * * @return the value of the fourth component */ - T w() const { + template + T w(typename std::enable_if= 4>::type* = 0) const { static_assert(S > 3); return v[3]; }