From cc742ee755de3100114a5002e5f6ddd34ed92e7b Mon Sep 17 00:00:00 2001 From: Philip Top Date: Wed, 29 Jul 2026 07:54:54 -0700 Subject: [PATCH 1/2] update templates to remove possible leak --- src/core/ObjectFactoryTemplates.hpp | 53 ++++---- src/optimization/optObjectFactory.h | 31 ++++- test/componentTests/simulationTests.cpp | 159 ++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 24 deletions(-) diff --git a/src/core/ObjectFactoryTemplates.hpp b/src/core/ObjectFactoryTemplates.hpp index b059bfd56..62b13ec82 100644 --- a/src/core/ObjectFactoryTemplates.hpp +++ b/src/core/ObjectFactoryTemplates.hpp @@ -40,7 +40,7 @@ class GridObjectHolder: public CoreObject { ~GridObjectHolder() { for (auto& so : objArray) { - if (so.getParent()) { + if ((so.getParent() != nullptr) && (so.getParent()->getID() > 0)) { so.getParent()->remove(&so); } } @@ -87,29 +87,40 @@ class ObjectPrepper { ObjectPrepper(count_t objCount, CoreObject* example) { prepObjects(objCount, example); } void prepObjects(count_t objCount, CoreObject* example) { + if ((objCount == 0) || (example == nullptr)) { + useBlock = false; + return; + } + auto root = example->getRoot(); + if (root == nullptr) { + useBlock = false; + return; + } + useBlock = true; - if ((obptr) && (root != nullptr)) { - if (obptr->getParent() != root) { - root->add(obptr.get()); - } + if ((obptr) && (obptr->getParent() != root)) { + root->add(obptr.get()); } - if (remaining() < objCount) { - if ((obptr) && (obptr->remaining() > 0)) { - targetprepped = objCount - obptr->remaining(); - } else { - obptr = makeOwningPtr>(targetprepped); - if (root != nullptr) { - if (!obptr) { - root->log(root, PrintLevel::WARNING, "unable to create container object"); - useBlock = false; - } else { - root->add(obptr.get()); - } - } else { - useBlock = false; - } - } + + const auto availableObjects = remaining(); + if (availableObjects >= objCount) { + return; + } + + const auto objectDeficit = objCount - availableObjects; + if ((obptr) && (obptr->remaining() > 0)) { + targetprepped = objectDeficit; + return; + } + + targetprepped = 0; + obptr = makeOwningPtr>(objectDeficit); + if (!obptr) { + root->log(root, PrintLevel::WARNING, "unable to create container object"); + useBlock = false; + } else { + root->add(obptr.get()); } } Ntype* getNewObject(std::string_view objName = {}) diff --git a/src/optimization/optObjectFactory.h b/src/optimization/optObjectFactory.h index 6e5759247..0d8fb4f18 100644 --- a/src/optimization/optObjectFactory.h +++ b/src/optimization/optObjectFactory.h @@ -6,6 +6,7 @@ #pragma once +#include "core/CoreOwningPtr.hpp" #include "gridOptObjects.h" #include #include @@ -139,7 +140,14 @@ class OptObjectFactory: public OptFactory { private: bool mUseBlock = false; - GridOptObjectHolder* mObjectHolder = nullptr; + CoreOwningPtr> mObjectHolder; + + void attachHolderToRoot(CoreObject* root) + { + if ((root != nullptr) && mObjectHolder && (mObjectHolder->getParent() != root)) { + root->add(mObjectHolder.get()); + } + } public: OptObjectFactory(std::string_view component, @@ -232,9 +240,26 @@ class OptObjectFactory: public OptFactory { virtual void prepObjects(count_t count, CoreObject* obj) override { + if ((count == 0) || (obj == nullptr)) { + mUseBlock = false; + return; + } + auto root = obj->getRoot(); - mObjectHolder = new GridOptObjectHolder(count); - root->add(mObjectHolder); + if (root == nullptr) { + mUseBlock = false; + mObjectHolder = nullptr; + return; + } + + if (mObjectHolder && (mObjectHolder->remaining() >= count)) { + attachHolderToRoot(root); + mUseBlock = true; + return; + } + + mObjectHolder = makeOwningPtr>(count); + attachHolderToRoot(root); mUseBlock = true; } virtual count_t remainingPrepped() const override diff --git a/test/componentTests/simulationTests.cpp b/test/componentTests/simulationTests.cpp index e85fb4aeb..93653189d 100644 --- a/test/componentTests/simulationTests.cpp +++ b/test/componentTests/simulationTests.cpp @@ -5,14 +5,173 @@ */ #include "../gtestHelper.h" +#include "core/ObjectFactoryTemplates.hpp" #include "gmlc/utilities/vectorOps.hpp" +#include "griddyn/griddyn-config.h" +#include #include #include #include #include #include #include +#include + +#ifdef GRIDDYN_ENABLE_OPTIMIZATION_LIBRARY +# include "optimization/optObjectFactory.h" +#endif class SimulationTests: public GridDynSimulationTestFixture, public ::testing::Test {}; TEST_F(SimulationTests, SimulationOrderingTests) {} + +namespace { +class CountingRootObject: public griddyn::CoreObject { + public: + int addCount = 0; + std::vector addedObjects; + + void add(griddyn::CoreObject* obj) override + { + if (obj == nullptr) { + return; + } + + ++addCount; + obj->addOwningReference(); + obj->setParent(this); + addedObjects.push_back(obj); + } + + void remove(griddyn::CoreObject* obj) override + { + const auto foundObject = std::find(addedObjects.begin(), addedObjects.end(), obj); + if (foundObject == addedObjects.end()) { + return; + } + + griddyn::removeReference(*foundObject, this); + addedObjects.erase(foundObject); + } + + ~CountingRootObject() override + { + for (auto* obj : addedObjects) { + griddyn::removeReference(obj, this); + } + } +}; + +class FactoryTestGridObject: public griddyn::CoreObject { + public: + FactoryTestGridObject() = default; + explicit FactoryTestGridObject(const std::string& objectName): CoreObject(objectName) {} +}; +} // namespace + +TEST(CoreFactoryTests, PrepObjectsIgnoresInvalidRequests) +{ + CountingRootObject root; + griddyn::TypeFactory factory("core-factory-prep-invalid-test", "object"); + + factory.prepObjects(0, &root); + EXPECT_EQ(root.addCount, 0); + EXPECT_EQ(factory.remainingPrepped(), 0U); + + factory.prepObjects(2, nullptr); + EXPECT_EQ(root.addCount, 0); + EXPECT_EQ(factory.remainingPrepped(), 0U); +} + +TEST(CoreFactoryTests, PrepObjectsCreatesNonEmptyHolderOnce) +{ + CountingRootObject root; + griddyn::TypeFactory factory("core-factory-prep-test", "object"); + + factory.prepObjects(3, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 3U); + + factory.prepObjects(2, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 3U); + + auto* object = factory.makeTypeObject(); + ASSERT_NE(object, nullptr); + EXPECT_EQ(factory.remainingPrepped(), 2U); + + factory.prepObjects(2, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 2U); + + factory.prepObjects(5, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 5U); + + factory.makeTypeObject(); + factory.makeTypeObject(); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 3U); + + factory.makeTypeObject(); + EXPECT_EQ(root.addCount, 2); + EXPECT_EQ(factory.remainingPrepped(), 2U); +} + +#ifdef GRIDDYN_ENABLE_OPTIMIZATION_LIBRARY +namespace { +class FactoryTestOptObject: public griddyn::GridOptObject { + public: + FactoryTestOptObject() = default; + explicit FactoryTestOptObject(griddyn::CoreObject* obj) { add(obj); } + + griddyn::CoreObject* sourceObject = nullptr; + + void add(griddyn::CoreObject* obj) override { sourceObject = obj; } +}; +} // namespace + +TEST(OptimizationFactoryTests, PrepObjectsReusesAttachedHolder) +{ + CountingRootObject root; + FactoryTestGridObject gridObject; + griddyn::OptObjectFactory factory( + "factory-prep-test", "object"); + + factory.prepObjects(3, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 3U); + + factory.prepObjects(2, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 3U); + + auto* optObject = factory.makeTypeObject(&gridObject); + ASSERT_NE(optObject, nullptr); + EXPECT_EQ(optObject->sourceObject, &gridObject); + EXPECT_EQ(factory.remainingPrepped(), 2U); + + factory.prepObjects(2, &root); + EXPECT_EQ(root.addCount, 1); + EXPECT_EQ(factory.remainingPrepped(), 2U); + + factory.prepObjects(3, &root); + EXPECT_EQ(root.addCount, 2); + EXPECT_EQ(factory.remainingPrepped(), 3U); +} + +TEST(OptimizationFactoryTests, PrepObjectsIgnoresInvalidRequests) +{ + CountingRootObject root; + griddyn::OptObjectFactory factory( + "factory-prep-invalid-test", "object"); + + factory.prepObjects(0, &root); + EXPECT_EQ(root.addCount, 0); + EXPECT_EQ(factory.remainingPrepped(), 0U); + + factory.prepObjects(2, nullptr); + EXPECT_EQ(root.addCount, 0); + EXPECT_EQ(factory.remainingPrepped(), 0U); +} +#endif From eb15ac98a2e8c4b04fb7266302d94ace8dfac197 Mon Sep 17 00:00:00 2001 From: Philip Top Date: Wed, 29 Jul 2026 18:03:56 -0700 Subject: [PATCH 2/2] update for clang-tidy --- test/componentTests/simulationTests.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/componentTests/simulationTests.cpp b/test/componentTests/simulationTests.cpp index 93653189d..16f917337 100644 --- a/test/componentTests/simulationTests.cpp +++ b/test/componentTests/simulationTests.cpp @@ -5,6 +5,7 @@ */ #include "../gtestHelper.h" +#include "core/CoreOwningPtr.hpp" #include "core/ObjectFactoryTemplates.hpp" #include "gmlc/utilities/vectorOps.hpp" #include "griddyn/griddyn-config.h" @@ -96,8 +97,8 @@ TEST(CoreFactoryTests, PrepObjectsCreatesNonEmptyHolderOnce) EXPECT_EQ(root.addCount, 1); EXPECT_EQ(factory.remainingPrepped(), 3U); - auto* object = factory.makeTypeObject(); - ASSERT_NE(object, nullptr); + griddyn::CoreOwningPtr object{factory.makeTypeObject()}; + ASSERT_TRUE(static_cast(object)); EXPECT_EQ(factory.remainingPrepped(), 2U); factory.prepObjects(2, &root); @@ -108,12 +109,13 @@ TEST(CoreFactoryTests, PrepObjectsCreatesNonEmptyHolderOnce) EXPECT_EQ(root.addCount, 1); EXPECT_EQ(factory.remainingPrepped(), 5U); - factory.makeTypeObject(); - factory.makeTypeObject(); + std::vector> objects; + objects.emplace_back(factory.makeTypeObject()); + objects.emplace_back(factory.makeTypeObject()); EXPECT_EQ(root.addCount, 1); EXPECT_EQ(factory.remainingPrepped(), 3U); - factory.makeTypeObject(); + objects.emplace_back(factory.makeTypeObject()); EXPECT_EQ(root.addCount, 2); EXPECT_EQ(factory.remainingPrepped(), 2U); } @@ -123,7 +125,7 @@ namespace { class FactoryTestOptObject: public griddyn::GridOptObject { public: FactoryTestOptObject() = default; - explicit FactoryTestOptObject(griddyn::CoreObject* obj) { add(obj); } + explicit FactoryTestOptObject(griddyn::CoreObject* obj): sourceObject(obj) {} griddyn::CoreObject* sourceObject = nullptr; @@ -146,8 +148,8 @@ TEST(OptimizationFactoryTests, PrepObjectsReusesAttachedHolder) EXPECT_EQ(root.addCount, 1); EXPECT_EQ(factory.remainingPrepped(), 3U); - auto* optObject = factory.makeTypeObject(&gridObject); - ASSERT_NE(optObject, nullptr); + griddyn::CoreOwningPtr optObject{factory.makeTypeObject(&gridObject)}; + ASSERT_TRUE(static_cast(optObject)); EXPECT_EQ(optObject->sourceObject, &gridObject); EXPECT_EQ(factory.remainingPrepped(), 2U);