From 2a386c9f6f8b2cd87b196b5c8e8447de8dd13a75 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Tue, 12 Sep 2023 15:35:17 -0500 Subject: [PATCH 1/4] App/Toponaming: Add StringHasher to Document --- src/App/Document.cpp | 58 ++++++++++++++++++++++++++++++++++- src/App/Document.h | 36 ++++++++++++++++++++++ src/App/Property.h | 2 ++ src/App/PropertyContainer.cpp | 17 ++++++++++ src/App/PropertyContainer.h | 1 + src/App/private/DocumentP.h | 7 +++++ tests/src/App/Document.cpp | 3 ++ 7 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 tests/src/App/Document.cpp diff --git a/src/App/Document.cpp b/src/App/Document.cpp index 54b6d79f06..9b9613d7f8 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -64,6 +64,7 @@ recompute path. Also, it enables more complicated dependencies beyond trees. #endif #include +#include #include #ifdef USE_OLD_DAG @@ -102,6 +103,7 @@ recompute path. Also, it enables more complicated dependencies beyond trees. #include "License.h" #include "Link.h" #include "MergeDocuments.h" +#include "StringHasher.h" #include "Transactions.h" #ifdef _MSC_VER @@ -136,6 +138,7 @@ static bool globalIsRelabeling; DocumentP::DocumentP() { + Hasher = new StringHasher; static std::random_device _RD; static std::mt19937 _RGEN(_RD()); static std::uniform_int_distribution<> _RDIST(0, 5000); @@ -921,11 +924,27 @@ std::string Document::getTransientDirectoryName(const std::string& uuid, const s void Document::Save (Base::Writer &writer) const { + d->hashers.clear(); + addStringHasher(d->Hasher); + writer.Stream() << R"(" << endl; + << "\" FileVersion=\"" << writer.getFileVersion() + << "\" StringHasher=\"1\">\n"; + + writer.incInd(); + + d->Hasher->setPersistenceFileName("StringHasher.Table"); + for (auto o : d->objectArray) { + o->beforeSave(); + } + beforeSave(); + + d->Hasher->Save(writer); + + writer.decInd(); PropertyContainer::Save(writer); @@ -937,7 +956,9 @@ void Document::Save (Base::Writer &writer) const void Document::Restore(Base::XMLReader &reader) { int i,Cnt; + d->hashers.clear(); d->touchedObjs.clear(); + addStringHasher(d->Hasher); setStatus(Document::PartialDoc,false); reader.readElement("Document"); @@ -954,6 +975,12 @@ void Document::Restore(Base::XMLReader &reader) reader.FileVersion = 0; } + if (reader.hasAttribute("StringHasher")) { + d->Hasher->Restore(reader); + } else { + d->Hasher->clear(); + } + // When this document was created the FileName and Label properties // were set to the absolute path or file name, respectively. To save // the document to the file it was loaded from or to show the file name @@ -1018,6 +1045,29 @@ void Document::Restore(Base::XMLReader &reader) reader.readEndElement("Document"); } +std::pair Document::addStringHasher(const StringHasherRef & hasher) const { + if (!hasher) + return std::make_pair(false, 0); + auto ret = d->hashers.left.insert(HasherMap::left_map::value_type(hasher,(int)d->hashers.size())); + if (ret.second) + hasher->clearMarks(); + return std::make_pair(ret.second,ret.first->second); +} + +StringHasherRef Document::getStringHasher(int idx) const { + if(idx<0) { + return d->Hasher; + } + StringHasherRef hasher; + auto it = d->hashers.right.find(idx); + if(it == d->hashers.right.end()) { + hasher = new StringHasher; + d->hashers.right.insert(HasherMap::right_map::value_type(idx,hasher)); + }else + hasher = it->second; + return hasher; +} + struct DocExportStatus { Document::ExportStatus status; std::set objs; @@ -1054,6 +1104,7 @@ Document::ExportStatus Document::isExporting(const App::DocumentObject *obj) con void Document::exportObjects(const std::vector& obj, std::ostream& out) { DocumentExporting exporting(obj); + d->hashers.clear(); if(FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) { for(auto o : obj) { @@ -1090,6 +1141,7 @@ void Document::exportObjects(const std::vector& obj, std:: // write additional files writer.writeFiles(); + d->hashers.clear(); } #define FC_ATTR_DEPENDENCIES "Dependencies" @@ -1401,6 +1453,7 @@ void Document::addRecomputeObject(DocumentObject *obj) { std::vector Document::importObjects(Base::XMLReader& reader) { + d->hashers.clear(); Base::FlagToggler<> flag(globalIsRestoring, false); Base::ObjectStatusLocker restoreBit(Status::Restoring, this); Base::ObjectStatusLocker restoreBit2(Status::Importing, this); @@ -1452,6 +1505,7 @@ Document::importObjects(Base::XMLReader& reader) o->setStatus(App::ObjImporting,false); } + d->hashers.clear(); return objs; } @@ -1464,6 +1518,8 @@ unsigned int Document::getMemSize () const for (it = d->objectArray.begin(); it != d->objectArray.end(); ++it) size += (*it)->getMemSize(); + size += d->Hasher->getMemSize(); + // size of the document properties... size += PropertyContainer::getMemSize(); diff --git a/src/App/Document.h b/src/App/Document.h index ad202a31d3..ec8bfbd331 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -23,6 +23,12 @@ #ifndef APP_DOCUMENT_H #define APP_DOCUMENT_H +#include +#include +#include +#include +#include + #include "PropertyContainer.h" #include "PropertyLinks.h" #include "PropertyStandard.h" @@ -44,6 +50,8 @@ namespace App class DocumentPy; // the python document class class Application; class Transaction; + class StringHasher; + typedef Base::Reference StringHasherRef; } namespace App @@ -465,6 +473,34 @@ public: (const App::DocumentObject* from, const App::DocumentObject* to) const; //@} + /** Called by property during properly save its containing StringHasher + * + * @param hasher: the input hasher + * @return Returns a pair. Boolean member indicate if the + * StringHasher has been saved before. The Integer is the hasher index. + * + * The StringHasher object is designed to be shared among multiple objects. + * So, we must not save duplicate copies of the same hasher. And must be + * able to restore with the same sharing relationship. This function returns + * whether the hasher has been saved before by other objects, and the index + * of the hasher. If the hasher has not been saved before, the object must + * save the hasher by calling StringHasher::Save + */ + std::pair addStringHasher(const StringHasherRef & hasher) const; + + /** Called by property to restore its containing StringHasher + * + * @param index: the index previously returned by calling addStringHasher() + * during save. Or if is negative, then return document's own string hasher. + * + * @return Return the resulting string hasher. + * + * The caller is responsible to restore the hasher itself if it is the first + * owner of the hasher, i.e. return addStringHasher() returns true during + * save + */ + StringHasherRef getStringHasher(int index=-1) const; + /** Return the links to a given object * * @param links: holds the links found diff --git a/src/App/Property.h b/src/App/Property.h index 8957956124..59c6ff505b 100644 --- a/src/App/Property.h +++ b/src/App/Property.h @@ -261,6 +261,8 @@ public: */ int64_t getID() const {return _id;} + virtual void beforeSave() const {} + friend class PropertyContainer; friend struct PropertyData; friend class DynamicProperty; diff --git a/src/App/PropertyContainer.cpp b/src/App/PropertyContainer.cpp index 719c124699..ce320e55a4 100644 --- a/src/App/PropertyContainer.cpp +++ b/src/App/PropertyContainer.cpp @@ -219,6 +219,23 @@ void PropertyContainer::handleChangedPropertyType(XMLReader &reader, const char PropertyData PropertyContainer::propertyData; +void PropertyContainer::beforeSave() const +{ + std::map Map; + getPropertyMap(Map); + for(auto &entry : Map) { + auto prop = entry.second; + if(!prop->testStatus(Property::PropDynamic) + && (prop->testStatus(Property::Transient) || + getPropertyType(prop) & Prop_Transient)) + { + // Nothing + } else { + prop->beforeSave(); + } + } +} + void PropertyContainer::Save (Base::Writer &writer) const { std::map Map; diff --git a/src/App/PropertyContainer.h b/src/App/PropertyContainer.h index 03d9fbe562..17a2102efc 100644 --- a/src/App/PropertyContainer.h +++ b/src/App/PropertyContainer.h @@ -220,6 +220,7 @@ public: void Save (Base::Writer &writer) const override; void Restore(Base::XMLReader &reader) override; + virtual void beforeSave() const; virtual void editProperty(const char * /*propName*/) {} diff --git a/src/App/private/DocumentP.h b/src/App/private/DocumentP.h index a065ce7141..3246459f53 100644 --- a/src/App/private/DocumentP.h +++ b/src/App/private/DocumentP.h @@ -25,11 +25,14 @@ #include #include +#include #include +#include #include #include #include + // using VertexProperty = boost::property; using DependencyList = boost::adjacency_list < boost::vecS, // class OutEdgeListS : a Sequence or an AssociativeContainer @@ -47,6 +50,7 @@ using Node = std::vector ; using Path = std::vector ; namespace App { +typedef boost::bimap HasherMap; class Transaction; // Pimpl class @@ -74,6 +78,7 @@ struct DocumentP unsigned int UndoMemSize; unsigned int UndoMaxStackSize; std::string programVersion; + mutable HasherMap hashers; #ifdef USE_OLD_DAG DependencyList DepList; std::map VertexObjectList; @@ -82,6 +87,8 @@ struct DocumentP std::multimap > _RecomputeLog; + StringHasherRef Hasher; + DocumentP(); void addRecomputeLog(const char *why, App::DocumentObject *obj) { diff --git a/tests/src/App/Document.cpp b/tests/src/App/Document.cpp new file mode 100644 index 0000000000..b6d9e89d67 --- /dev/null +++ b/tests/src/App/Document.cpp @@ -0,0 +1,3 @@ +// +// Created by Chris Hennes on 9/13/23. +// From b8055fa6f4768afad8a133ff565d5e8fbf5faed4 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Wed, 13 Sep 2023 09:25:22 -0500 Subject: [PATCH 2/4] App/Toponaming: Minor code cleanup --- src/App/Document.cpp | 1 + src/App/Document.h | 13 ++++++------- src/App/PropertyContainer.cpp | 17 +++++++++-------- src/App/private/DocumentP.h | 2 +- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/App/Document.cpp b/src/App/Document.cpp index 9b9613d7f8..22768f46fe 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -1056,6 +1056,7 @@ std::pair Document::addStringHasher(const StringHasherRef & hasher) co StringHasherRef Document::getStringHasher(int idx) const { if(idx<0) { + return d->Hasher; return d->Hasher; } StringHasherRef hasher; diff --git a/src/App/Document.h b/src/App/Document.h index ec8bfbd331..4e4d65e042 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -51,7 +51,7 @@ namespace App class Application; class Transaction; class StringHasher; - typedef Base::Reference StringHasherRef; + using StringHasherRef = Base::Reference; } namespace App @@ -476,11 +476,11 @@ public: /** Called by property during properly save its containing StringHasher * * @param hasher: the input hasher - * @return Returns a pair. Boolean member indicate if the - * StringHasher has been saved before. The Integer is the hasher index. + * @return Returns a pair. The boolean indicates if the + * StringHasher has been saved before. The integer is the hasher index. * * The StringHasher object is designed to be shared among multiple objects. - * So, we must not save duplicate copies of the same hasher. And must be + * We must not save duplicate copies of the same hasher, and must be * able to restore with the same sharing relationship. This function returns * whether the hasher has been saved before by other objects, and the index * of the hasher. If the hasher has not been saved before, the object must @@ -495,9 +495,8 @@ public: * * @return Return the resulting string hasher. * - * The caller is responsible to restore the hasher itself if it is the first - * owner of the hasher, i.e. return addStringHasher() returns true during - * save + * The caller is responsible for restoring the hasher if the caller is the first + * owner of the hasher, i.e. if addStringHasher() returns true during save. */ StringHasherRef getStringHasher(int index=-1) const; diff --git a/src/App/PropertyContainer.cpp b/src/App/PropertyContainer.cpp index ce320e55a4..e4f76e92aa 100644 --- a/src/App/PropertyContainer.cpp +++ b/src/App/PropertyContainer.cpp @@ -221,16 +221,16 @@ PropertyData PropertyContainer::propertyData; void PropertyContainer::beforeSave() const { - std::map Map; + std::map Map; getPropertyMap(Map); - for(auto &entry : Map) { + for (auto& entry : Map) { auto prop = entry.second; - if(!prop->testStatus(Property::PropDynamic) - && (prop->testStatus(Property::Transient) || - getPropertyType(prop) & Prop_Transient)) - { + if (!prop->testStatus(Property::PropDynamic) + && (prop->testStatus(Property::Transient) + || ((getPropertyType(prop) & Prop_Transient) != 0))) { // Nothing - } else { + } + else { prop->beforeSave(); } } @@ -254,8 +254,9 @@ void PropertyContainer::Save (Base::Writer &writer) const { transients.push_back(prop); it = Map.erase(it); - }else + } else { ++it; + } } writer.incInd(); // indentation for 'Properties Count' diff --git a/src/App/private/DocumentP.h b/src/App/private/DocumentP.h index 3246459f53..a85d90af50 100644 --- a/src/App/private/DocumentP.h +++ b/src/App/private/DocumentP.h @@ -50,7 +50,7 @@ using Node = std::vector ; using Path = std::vector ; namespace App { -typedef boost::bimap HasherMap; +using HasherMap = boost::bimap; class Transaction; // Pimpl class From ef91d82daffb584a2cf7eae43ec318875aea9597 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Thu, 14 Sep 2023 10:50:02 -0500 Subject: [PATCH 3/4] App/Toponaming: Add a few tests for Document --- tests/src/App/CMakeLists.txt | 1 + tests/src/App/Document.cpp | 104 ++++++++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/tests/src/App/CMakeLists.txt b/tests/src/App/CMakeLists.txt index 2e3019782a..14f763c1a8 100644 --- a/tests/src/App/CMakeLists.txt +++ b/tests/src/App/CMakeLists.txt @@ -4,6 +4,7 @@ target_sources( ${CMAKE_CURRENT_SOURCE_DIR}/Application.cpp ${CMAKE_CURRENT_SOURCE_DIR}/Branding.cpp ${CMAKE_CURRENT_SOURCE_DIR}/ComplexGeoData.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/Document.cpp ${CMAKE_CURRENT_SOURCE_DIR}/Expression.cpp ${CMAKE_CURRENT_SOURCE_DIR}/ElementMap.cpp ${CMAKE_CURRENT_SOURCE_DIR}/IndexedName.cpp diff --git a/tests/src/App/Document.cpp b/tests/src/App/Document.cpp index b6d9e89d67..027c60c583 100644 --- a/tests/src/App/Document.cpp +++ b/tests/src/App/Document.cpp @@ -1,3 +1,101 @@ -// -// Created by Chris Hennes on 9/13/23. -// +// SPDX-License-Identifier: LGPL-2.1-or-later + +#include "gtest/gtest.h" +#include + +#include "App/Application.h" +#include "App/Document.h" +#include "App/StringHasher.h" +#include "Base/Writer.h" + +using ::testing::Eq; +using ::testing::Ne; + +// NOLINTBEGIN(readability-magic-numbers) + +class FakeWriter: public Base::Writer +{ + void writeFiles() override + {} + std::ostream& Stream() override + { + return std::cout; + } +}; + +class DocumentTest: public ::testing::Test +{ +protected: + static void SetUpTestSuite() + { + if (App::Application::GetARGC() == 0) { + constexpr int argc = 1; + std::array argv {"FreeCAD"}; + App::Application::Config()["ExeName"] = "FreeCAD"; + App::Application::init(argc, const_cast(argv.data())); // NOLINT + } + } + + void SetUp() override + { + _docName = App::GetApplication().getUniqueDocumentName("test"); + _doc = App::GetApplication().newDocument(_docName.c_str(), "testUser"); + } + + void TearDown() override + { + App::GetApplication().closeDocument(_docName.c_str()); + } + + App::Document* doc() + { + return _doc; + } + +private: + std::string _docName; + App::Document* _doc {}; +}; + + +TEST_F(DocumentTest, addStringHasherIndicatesUnwrittenWhenNew) +{ + // Arrange + App::StringHasherRef hasher(new App::StringHasher); + + // Act + auto addResult = doc()->addStringHasher(hasher); + + // Assert + EXPECT_TRUE(addResult.first); + EXPECT_THAT(addResult.second, Ne(-1)); +} + +TEST_F(DocumentTest, addStringHasherIndicatesAlreadyWritten) +{ + // Arrange + App::StringHasherRef hasher(new App::StringHasher); + doc()->addStringHasher(hasher); + + // Act + auto addResult = doc()->addStringHasher(hasher); + + // Assert + EXPECT_FALSE(addResult.first); +} + +TEST_F(DocumentTest, getStringHasherGivesExpectedHasher) +{ + // Arrange + App::StringHasherRef hasher(new App::StringHasher); + auto pair = doc()->addStringHasher(hasher); + int index = pair.second; + + // Act + auto foundHasher = doc()->getStringHasher(index); + + // Assert + EXPECT_EQ(hasher, foundHasher); +} + +// NOLINTEND(readability-magic-numbers) From 6704cc6aff76eca7e6bdfdd96c46b31238334458 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Sun, 17 Sep 2023 13:46:19 -0500 Subject: [PATCH 4/4] App/Toponaming: Clarify return value of addStringHasher --- src/App/Document.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/App/Document.h b/src/App/Document.h index 4e4d65e042..1f89b74ec5 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -473,22 +473,22 @@ public: (const App::DocumentObject* from, const App::DocumentObject* to) const; //@} - /** Called by property during properly save its containing StringHasher + /** Called by a property during save to store its StringHasher * * @param hasher: the input hasher * @return Returns a pair. The boolean indicates if the - * StringHasher has been saved before. The integer is the hasher index. + * StringHasher has been added before. The integer is the hasher index. * * The StringHasher object is designed to be shared among multiple objects. * We must not save duplicate copies of the same hasher, and must be * able to restore with the same sharing relationship. This function returns - * whether the hasher has been saved before by other objects, and the index - * of the hasher. If the hasher has not been saved before, the object must + * whether the hasher has been added before by other objects, and the index + * of the hasher. If the hasher has not been added before, the object must * save the hasher by calling StringHasher::Save */ std::pair addStringHasher(const StringHasherRef & hasher) const; - /** Called by property to restore its containing StringHasher + /** Called by property to restore its StringHasher * * @param index: the index previously returned by calling addStringHasher() * during save. Or if is negative, then return document's own string hasher.