From 320d06239e5a4432bdcb5e3a65529516683cf312 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Wed, 15 Mar 2023 09:24:50 -0500 Subject: [PATCH 1/3] App/Toponaming: Add MappedElement class --- src/App/CMakeLists.txt | 2 +- src/App/MappedElement.cpp | 5 ++ src/App/MappedElement.h | 148 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 src/App/MappedElement.cpp create mode 100644 src/App/MappedElement.h diff --git a/src/App/CMakeLists.txt b/src/App/CMakeLists.txt index bd9dbc4b3a..4baebeacc1 100644 --- a/src/App/CMakeLists.txt +++ b/src/App/CMakeLists.txt @@ -296,7 +296,7 @@ SET(FreeCADApp_SRCS FreeCADTest.py PreCompiled.cpp PreCompiled.h -) + MappedElement.cpp MappedElement.h) if(FREECAD_USE_PCH) add_definitions(-D_PreComp_) diff --git a/src/App/MappedElement.cpp b/src/App/MappedElement.cpp new file mode 100644 index 0000000000..62ba30a58b --- /dev/null +++ b/src/App/MappedElement.cpp @@ -0,0 +1,5 @@ +// +// Created by Chris Hennes on 3/14/23. +// + +#include "MappedElement.h" diff --git a/src/App/MappedElement.h b/src/App/MappedElement.h new file mode 100644 index 0000000000..948dbc834c --- /dev/null +++ b/src/App/MappedElement.h @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later + +/**************************************************************************** + * Copyright (c) 2022 Zheng, Lei (realthunder) * + * Copyright (c) 2023 FreeCAD Project Association * + * * + * This file is part of FreeCAD. * + * * + * FreeCAD is free software: you can redistribute it and/or modify it * + * under the terms of the GNU Lesser General Public License as * + * published by the Free Software Foundation, either version 2.1 of the * + * License, or (at your option) any later version. * + * * + * FreeCAD is distributed in the hope that it will be useful, but * + * WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * + * Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public * + * License along with FreeCAD. If not, see * + * . * + * * + ***************************************************************************/ + +#ifndef APP_MAPPEDELEMENT_H +#define APP_MAPPEDELEMENT_H + +#include +#include +#include +#include +#include +#include +#include "ComplexGeoData.h" +#include "IndexedName.h" +#include "MappedName.h" + +namespace App +{ +class DocumentObject; +} + +namespace Data +{ + +struct AppExport MappedElement +{ + IndexedName index; + MappedName name; + + MappedElement() = default; + + MappedElement(const IndexedName & idx, const MappedName & n) + : index(idx), name(n) + {} + + MappedElement(const MappedName & n, const IndexedName & idx) + : index(idx), name(n) + {} + + MappedElement(const MappedElement & other) + : index(other.index), name(other.name) + {} + + MappedElement(MappedElement && other) + : index(std::move(other.index)), name(std::move(other.name)) + {} + + MappedElement & operator=(MappedElement && other) + { + this->index = std::move(other.index); + this->name = std::move(other.name); + return *this; + } + + MappedElement & operator=(const MappedElement & other) + { + this->index = other.index; + this->name = other.name; + return *this; + } + + bool operator==(const MappedElement &other) const + { + return this->index == other.index && this->name == other.name; + } + + bool operator!=(const MappedElement &other) const + { + return this->index != other.index || this->name != other.name; + } + + bool operator<(const MappedElement &other) const + { + int res = this->index.compare(other.index); + if (res < 0) + return true; + if (res > 0) + return false; + return this->name < other.name; + } +}; + +struct AppExport HistoryItem { + App::DocumentObject *obj; + long tag; + Data::MappedName element; + Data::IndexedName index; + std::vector intermediates; + HistoryItem(App::DocumentObject *obj, const Data::MappedName &name); +}; + +struct AppExport ElementNameComp { + /** Comparison function to make topo name more stable + * + * The sorting decomposes the name into either of the following two forms + * '#' + hex_digits + tail + * non_digits + digits + tail + * + * The non-digits part is compared lexically, while the digits part is + * compared by its integer value. + * + * The reason for this is to prevent names with a larger index (which usually means that it + * comes later in the history) being sorted to before something with the same name but a larger + * index. + */ + bool operator()(const MappedName &a, const MappedName &b) const; +}; + +typedef QVector ElementIDRefs; + +struct AppExport MappedChildElements +{ + IndexedName indexedName; + int count; + int offset; + long tag; + ElementMapPtr elementMap; + QByteArray postfix; + ElementIDRefs sids; + + static const std::string & prefix(); +}; + +} //namespace Data + + +#endif// FREECAD_MAPPEDELEMENT_H From b4d41291dc8ecc41f270224dd2ab32c4a7c1c4ed Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Wed, 15 Mar 2023 19:41:33 -0500 Subject: [PATCH 2/3] App/Toponaming: clang-tidy cleanup of MappedElement --- src/App/MappedElement.h | 125 +++++++++++++--------------------------- 1 file changed, 40 insertions(+), 85 deletions(-) diff --git a/src/App/MappedElement.h b/src/App/MappedElement.h index 948dbc834c..2e4b99ffa1 100644 --- a/src/App/MappedElement.h +++ b/src/App/MappedElement.h @@ -1,29 +1,27 @@ // SPDX-License-Identifier: LGPL-2.1-or-later -/**************************************************************************** - * Copyright (c) 2022 Zheng, Lei (realthunder) * - * Copyright (c) 2023 FreeCAD Project Association * - * * - * This file is part of FreeCAD. * - * * - * FreeCAD is free software: you can redistribute it and/or modify it * - * under the terms of the GNU Lesser General Public License as * - * published by the Free Software Foundation, either version 2.1 of the * - * License, or (at your option) any later version. * - * * - * FreeCAD is distributed in the hope that it will be useful, but * - * WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * - * Lesser General Public License for more details. * - * * - * You should have received a copy of the GNU Lesser General Public * - * License along with FreeCAD. If not, see * - * . * - * * - ***************************************************************************/ +/*************************************************************************************************** + * * + * Copyright (c) 2022 Zheng, Lei (realthunder) * + * Copyright (c) 2023 FreeCAD Project Association * + * * + * This file is part of FreeCAD. * + * * + * FreeCAD is free software: you can redistribute it and/or modify it under the terms of the * + * GNU Lesser General Public License as published by the Free Software Foundation, either * + * version 2.1 of the License, or (at your option) any later version. * + * * + * FreeCAD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; * + * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. * + * See the GNU Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public License along with * + * FreeCAD. If not, see . * + * * + **************************************************************************************************/ -#ifndef APP_MAPPEDELEMENT_H -#define APP_MAPPEDELEMENT_H +#ifndef APP_MAPPED_ELEMENT_H +#define APP_MAPPED_ELEMENT_H #include #include @@ -31,6 +29,7 @@ #include #include #include +#include #include "ComplexGeoData.h" #include "IndexedName.h" #include "MappedName.h" @@ -50,35 +49,30 @@ struct AppExport MappedElement MappedElement() = default; - MappedElement(const IndexedName & idx, const MappedName & n) - : index(idx), name(n) + MappedElement(const IndexedName & idx, MappedName n) + : index(idx), name(std::move(n)) {} - MappedElement(const MappedName & n, const IndexedName & idx) - : index(idx), name(n) + MappedElement(MappedName n, const IndexedName & idx) + : index(idx), name(std::move(n)) {} - MappedElement(const MappedElement & other) - : index(other.index), name(other.name) + ~MappedElement() = default; + + MappedElement(const MappedElement & other) = default; + + MappedElement(MappedElement && other) noexcept + : index(other.index), name(std::move(other.name)) {} - MappedElement(MappedElement && other) - : index(std::move(other.index)), name(std::move(other.name)) - {} - - MappedElement & operator=(MappedElement && other) + MappedElement & operator=(MappedElement && other) noexcept { - this->index = std::move(other.index); + this->index = other.index; this->name = std::move(other.name); return *this; } - MappedElement & operator=(const MappedElement & other) - { - this->index = other.index; - this->name = other.name; - return *this; - } + MappedElement & operator=(const MappedElement & other) = default; bool operator==(const MappedElement &other) const { @@ -93,56 +87,17 @@ struct AppExport MappedElement bool operator<(const MappedElement &other) const { int res = this->index.compare(other.index); - if (res < 0) + if (res < 0) { return true; - if (res > 0) + } + if (res > 0) { return false; + } return this->name < other.name; } }; -struct AppExport HistoryItem { - App::DocumentObject *obj; - long tag; - Data::MappedName element; - Data::IndexedName index; - std::vector intermediates; - HistoryItem(App::DocumentObject *obj, const Data::MappedName &name); -}; - -struct AppExport ElementNameComp { - /** Comparison function to make topo name more stable - * - * The sorting decomposes the name into either of the following two forms - * '#' + hex_digits + tail - * non_digits + digits + tail - * - * The non-digits part is compared lexically, while the digits part is - * compared by its integer value. - * - * The reason for this is to prevent names with a larger index (which usually means that it - * comes later in the history) being sorted to before something with the same name but a larger - * index. - */ - bool operator()(const MappedName &a, const MappedName &b) const; -}; - -typedef QVector ElementIDRefs; - -struct AppExport MappedChildElements -{ - IndexedName indexedName; - int count; - int offset; - long tag; - ElementMapPtr elementMap; - QByteArray postfix; - ElementIDRefs sids; - - static const std::string & prefix(); -}; - } //namespace Data -#endif// FREECAD_MAPPEDELEMENT_H +#endif// APP_MAPPED_ELEMENT_H From d871779299cbd24e18a168f6f1e2a50fc85ceb70 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Thu, 16 Mar 2023 15:39:28 -0500 Subject: [PATCH 3/3] App/Toponaming: Add tests to MappedElement --- src/App/CMakeLists.txt | 4 +- src/App/MappedElement.cpp | 25 ++++++- src/App/MappedElement.h | 42 ++++++----- tests/src/App/CMakeLists.txt | 1 + tests/src/App/MappedElement.cpp | 129 ++++++++++++++++++++++++++++++++ 5 files changed, 177 insertions(+), 24 deletions(-) create mode 100644 tests/src/App/MappedElement.cpp diff --git a/src/App/CMakeLists.txt b/src/App/CMakeLists.txt index 4baebeacc1..4326b562f0 100644 --- a/src/App/CMakeLists.txt +++ b/src/App/CMakeLists.txt @@ -264,6 +264,7 @@ SET(FreeCADApp_CPP_SRCS ComplexGeoDataPyImp.cpp Enumeration.cpp IndexedName.cpp + MappedElement.cpp MappedName.cpp Material.cpp MaterialPyImp.cpp @@ -284,6 +285,7 @@ SET(FreeCADApp_HPP_SRCS Enumeration.h IndexedName.h MappedName.h + MappedElement.h Material.h Metadata.h ) @@ -296,7 +298,7 @@ SET(FreeCADApp_SRCS FreeCADTest.py PreCompiled.cpp PreCompiled.h - MappedElement.cpp MappedElement.h) +) if(FREECAD_USE_PCH) add_definitions(-D_PreComp_) diff --git a/src/App/MappedElement.cpp b/src/App/MappedElement.cpp index 62ba30a58b..14eb6f99ba 100644 --- a/src/App/MappedElement.cpp +++ b/src/App/MappedElement.cpp @@ -1,5 +1,24 @@ -// -// Created by Chris Hennes on 3/14/23. -// +// SPDX-License-Identifier: LGPL-2.1-or-later +/*************************************************************************************************** + * * + * Copyright (c) 2022 Zheng, Lei (realthunder) * + * Copyright (c) 2023 FreeCAD Project Association * + * * + * This file is part of FreeCAD. * + * * + * FreeCAD is free software: you can redistribute it and/or modify it under the terms of the * + * GNU Lesser General Public License as published by the Free Software Foundation, either * + * version 2.1 of the License, or (at your option) any later version. * + * * + * FreeCAD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; * + * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. * + * See the GNU Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public License along with * + * FreeCAD. If not, see . * + * * + **************************************************************************************************/ + +#include "PreCompiled.h" #include "MappedElement.h" diff --git a/src/App/MappedElement.h b/src/App/MappedElement.h index 2e4b99ffa1..ef5784d7d9 100644 --- a/src/App/MappedElement.h +++ b/src/App/MappedElement.h @@ -23,13 +23,6 @@ #ifndef APP_MAPPED_ELEMENT_H #define APP_MAPPED_ELEMENT_H -#include -#include -#include -#include -#include -#include -#include #include "ComplexGeoData.h" #include "IndexedName.h" #include "MappedName.h" @@ -42,6 +35,9 @@ class DocumentObject; namespace Data { +/// A MappedElement combines a MappedName and and IndexedName into a single entity and provides +/// simple comparison operators for the combination (including operator< so that the entity can +/// be sorted, or used in sorted containers). struct AppExport MappedElement { IndexedName index; @@ -49,42 +45,48 @@ struct AppExport MappedElement MappedElement() = default; - MappedElement(const IndexedName & idx, MappedName n) - : index(idx), name(std::move(n)) + MappedElement(const IndexedName& idx, MappedName n) + : index(idx), + name(std::move(n)) {} - MappedElement(MappedName n, const IndexedName & idx) - : index(idx), name(std::move(n)) + MappedElement(MappedName n, const IndexedName& idx) + : index(idx), + name(std::move(n)) {} ~MappedElement() = default; - MappedElement(const MappedElement & other) = default; + MappedElement(const MappedElement& other) = default; - MappedElement(MappedElement && other) noexcept - : index(other.index), name(std::move(other.name)) + MappedElement(MappedElement&& other) noexcept + : index(other.index), + name(std::move(other.name)) {} - MappedElement & operator=(MappedElement && other) noexcept + MappedElement& operator=(MappedElement&& other) noexcept { this->index = other.index; this->name = std::move(other.name); return *this; } - MappedElement & operator=(const MappedElement & other) = default; + MappedElement& operator=(const MappedElement& other) = default; - bool operator==(const MappedElement &other) const + bool operator==(const MappedElement& other) const { return this->index == other.index && this->name == other.name; } - bool operator!=(const MappedElement &other) const + bool operator!=(const MappedElement& other) const { return this->index != other.index || this->name != other.name; } - bool operator<(const MappedElement &other) const + /// For sorting purposes, one MappedElement is considered "less" than another if its index + /// compares less (which is first alphabetical, and then by numeric index). If the index of this + /// MappedElement is the same, then the names are compared lexicographically. + bool operator<(const MappedElement& other) const { int res = this->index.compare(other.index); if (res < 0) { @@ -97,7 +99,7 @@ struct AppExport MappedElement } }; -} //namespace Data +}// namespace Data #endif// APP_MAPPED_ELEMENT_H diff --git a/tests/src/App/CMakeLists.txt b/tests/src/App/CMakeLists.txt index ac4f46bc81..e159b934e5 100644 --- a/tests/src/App/CMakeLists.txt +++ b/tests/src/App/CMakeLists.txt @@ -6,6 +6,7 @@ target_sources( ${CMAKE_CURRENT_SOURCE_DIR}/ElementMap.cpp ${CMAKE_CURRENT_SOURCE_DIR}/IndexedName.cpp ${CMAKE_CURRENT_SOURCE_DIR}/License.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/MappedElement.cpp ${CMAKE_CURRENT_SOURCE_DIR}/MappedName.cpp ${CMAKE_CURRENT_SOURCE_DIR}/Metadata.cpp ) diff --git a/tests/src/App/MappedElement.cpp b/tests/src/App/MappedElement.cpp new file mode 100644 index 0000000000..9fc87b31f1 --- /dev/null +++ b/tests/src/App/MappedElement.cpp @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later + +#include "gtest/gtest.h" + +#include "App/IndexedName.h" +#include "App/MappedElement.h" + +class MappedElementTest: public ::testing::Test +{ +protected: + // void SetUp() override {} + + // void TearDown() override {} + + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) + static Data::MappedElement givenMappedElement(const char* index, const char* name) + { + Data::IndexedName indexedName {index}; + Data::MappedName mappedName {name}; + return {indexedName, mappedName}; + } +}; + +TEST_F(MappedElementTest, constructFromNameAndIndex) +{ + // Arrange + Data::IndexedName indexedName {"EDGE1"}; + Data::MappedName mappedName {"OTHER_NAME"}; + + // Act + Data::MappedElement mappedElement {indexedName, mappedName}; + + // Assert + EXPECT_EQ(mappedElement.index, indexedName); + EXPECT_EQ(mappedElement.name, mappedName); +} + +TEST_F(MappedElementTest, moveConstructor) +{ + // Arrange + auto originalMappedElement = givenMappedElement("EDGE1", "OTHER_NAME"); + auto originalName = originalMappedElement.name; + auto originalIndex = originalMappedElement.index; + + // Act + Data::MappedElement newMappedElement {std::move(originalMappedElement)}; + + // Assert + EXPECT_EQ(originalName, newMappedElement.name); + EXPECT_EQ(originalIndex, newMappedElement.index); +} + +TEST_F(MappedElementTest, assignmentOperator) +{ + // Arrange + auto mappedElementA = givenMappedElement("EDGE1", "OTHER_NAME"); + auto mappedElementB = givenMappedElement("EDGE2", "ANOTHER_NAME"); + EXPECT_NE(mappedElementA, mappedElementB);// Verify test setup + + // Act + mappedElementA = mappedElementB; + + // Assert + EXPECT_EQ(mappedElementA, mappedElementB); +} + +TEST_F(MappedElementTest, moveAssignmentOperator) +{ + // Arrange + auto mappedElementA = givenMappedElement("EDGE1", "OTHER_NAME"); + auto mappedElementB = givenMappedElement("EDGE2", "ANOTHER_NAME"); + EXPECT_NE(mappedElementA, mappedElementB);// Verify test setup + + // Act + mappedElementA = std::move(mappedElementB); + + // Assert + EXPECT_EQ(mappedElementA.name.toString(), "ANOTHER_NAME"); + EXPECT_EQ(mappedElementA.index.toString(), "EDGE2"); +} + +TEST_F(MappedElementTest, equalityOperatorsWhenNotEqual) +{ + // Arrange + auto mappedElementA = givenMappedElement("EDGE1", "OTHER_NAME"); + auto mappedElementB = givenMappedElement("EDGE2", "ANOTHER_NAME"); + + // Act + bool aAndBAreEqual = mappedElementA == mappedElementB; + bool aAndBAreNotEqual = mappedElementA != mappedElementB; + + // Assert + EXPECT_TRUE(aAndBAreNotEqual); + EXPECT_FALSE(aAndBAreEqual); +} + +TEST_F(MappedElementTest, equalityOperatorsWhenEqual) +{ + // Arrange + auto mappedElementA = givenMappedElement("EDGE1", "OTHER_NAME"); + auto mappedElementB = givenMappedElement("EDGE1", "OTHER_NAME"); + + // Act + bool aAndBAreEqual = mappedElementA == mappedElementB; + bool aAndBAreNotEqual = mappedElementA != mappedElementB; + + // Assert + EXPECT_FALSE(aAndBAreNotEqual); + EXPECT_TRUE(aAndBAreEqual); +} + +TEST_F(MappedElementTest, lessThanOperator) +{ + // Index is compared first, then mappedName + + // Arrange + auto mappedElement1A = givenMappedElement("EDGE1", "A"); + auto mappedElement1B = givenMappedElement("EDGE1", "B"); + auto mappedElement2A = givenMappedElement("EDGE2", "A"); + auto mappedElement2B = givenMappedElement("EDGE2", "B"); + auto mappedElement2BDuplicate = givenMappedElement("EDGE2", "B"); + + // Act & Assert + EXPECT_TRUE(mappedElement1A < mappedElement1B); + EXPECT_TRUE(mappedElement1A < mappedElement2A); + EXPECT_TRUE(mappedElement1B < mappedElement2A); + EXPECT_FALSE(mappedElement2A < mappedElement1B); + EXPECT_FALSE(mappedElement2B < mappedElement2BDuplicate); +}