From 03ca52b0362b1dd0ef41ebee87055f5a2b094e92 Mon Sep 17 00:00:00 2001 From: Mark Olesen Date: Tue, 25 Jul 2023 15:45:23 +0200 Subject: [PATCH] ENH: PtrList, PtrDynList, HashPtrTable try_emplace() method - naming like std::map::try_emplace(), it behaves like emplace_set() if there is no element at the given location otherwise a no-op ENH: reuse existing HashPtrTable 'slot' when setting pointers - avoids extra HashTable operations --- .../test/Dictionary/Test-Dictionary.C | 26 +++---- .../test/HashPtrTable/Test-HashPtrTable.C | 41 +++++------ applications/test/ISLList/Test-ISLList.C | 23 +++--- applications/test/PtrList/Test-PtrList.C | 28 +++----- .../Test-PtrListDictionary.C | 23 +++--- .../HashTables/HashPtrTable/HashPtrTable.H | 16 +++++ .../HashTables/HashPtrTable/HashPtrTableI.H | 72 ++++++++++++++++--- .../PtrLists/PtrDynList/PtrDynList.H | 7 ++ .../PtrLists/PtrDynList/PtrDynListI.H | 18 ++++- .../containers/PtrLists/PtrList/PtrList.H | 8 +++ .../containers/PtrLists/PtrList/PtrListI.H | 13 ++++ 11 files changed, 190 insertions(+), 85 deletions(-) diff --git a/applications/test/Dictionary/Test-Dictionary.C b/applications/test/Dictionary/Test-Dictionary.C index d5087918ee..41d772ecb8 100644 --- a/applications/test/Dictionary/Test-Dictionary.C +++ b/applications/test/Dictionary/Test-Dictionary.C @@ -55,10 +55,7 @@ public: i_(i) {} - const word& keyword() const - { - return keyword_; - } + word& keyword() const noexcept { return keyword_; } friend Ostream& operator<<(Ostream& os, const ent& e) { @@ -74,28 +71,27 @@ class Scalar public: - Scalar() - : - data_(0) - {} + static bool verbose; - Scalar(scalar val) - : - data_(val) - {} + constexpr Scalar() noexcept : data_(0) {} + Scalar(scalar val) noexcept : data_(val) {} ~Scalar() { - Info<<"delete Scalar: " << data_ << endl; + if (verbose) Info<< "delete Scalar: " << data_ << endl; } - friend Ostream& operator<<(Ostream& os, const Scalar& val) + scalar value() const noexcept { return data_; } + scalar& value() noexcept { return data_; } + + friend Ostream& operator<<(Ostream& os, const Scalar& item) { - os << val.data_; + os << item.value(); return os; } }; +bool Scalar::verbose = true; // * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * // diff --git a/applications/test/HashPtrTable/Test-HashPtrTable.C b/applications/test/HashPtrTable/Test-HashPtrTable.C index c7deadc2f9..36cb464f75 100644 --- a/applications/test/HashPtrTable/Test-HashPtrTable.C +++ b/applications/test/HashPtrTable/Test-HashPtrTable.C @@ -44,38 +44,28 @@ class Scalar public: - Scalar() - : - data_(0) - {} + static bool verbose; - Scalar(scalar val) - : - data_(val) - {} + constexpr Scalar() noexcept : data_(0) {} + Scalar(scalar val) noexcept : data_(val) {} ~Scalar() { - Info<<"delete Scalar: " << data_ << endl; + if (verbose) Info<< "delete Scalar: " << data_ << endl; } - const scalar& value() const - { - return data_; - } + const scalar& value() const noexcept { return data_; } + scalar& value() noexcept { return data_; } - scalar& value() + friend Ostream& operator<<(Ostream& os, const Scalar& item) { - return data_; - } - - friend Ostream& operator<<(Ostream& os, const Scalar& val) - { - os << val.data_; + os << item.value(); return os; } }; +bool Scalar::verbose = true; + template void printTable(const HashPtrTable& table) @@ -129,6 +119,9 @@ int main() myTable.set("natlog", new double(2.718282)); myTable.insert("sqrt2", autoPtr::New(1.414214)); myTable.insert("euler", autoPtr::New(0.577216)); + myTable.set("def_0", nullptr); + myTable.emplace_set("def_1", 123); + myTable.emplace_set("def_2", 456); HashTable> myTable1; @@ -146,6 +139,14 @@ int main() Info<< "Initial table" << nl; printTable(myTable); + myTable.try_emplace("def_0", 1000); // was nullptr, now value + myTable.try_emplace("def_1", 1001); // no-op + myTable.try_emplace("def_2", 1002); // no-op; + myTable.try_emplace("def_3", 1003); // was non-existent, now value + + Info<< "after try_emplace" << nl; + printTable(myTable); + Info<< "print" << nl; Info<< myTable2 << nl; diff --git a/applications/test/ISLList/Test-ISLList.C b/applications/test/ISLList/Test-ISLList.C index 263925bf7e..c115a28445 100644 --- a/applications/test/ISLList/Test-ISLList.C +++ b/applications/test/ISLList/Test-ISLList.C @@ -46,26 +46,27 @@ class Scalar { public: + // static bool verbose; + scalar data_; - Scalar() - : - data_(0) - {} + Scalar() : data_(0) {} + Scalar(scalar val) : data_(val) {} - Scalar(scalar s) - : - data_(s) - {} + // ~Scalar() {} - friend Ostream& operator<<(Ostream& os, const Scalar& s) + scalar value() const noexcept { return data_; } + scalar& value() noexcept { return data_; } + + friend Ostream& operator<<(Ostream& os, const Scalar& item) { - os << s.data_; + os << item.value(); return os; } - }; +// bool Scalar::verbose = true; + // * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * // // Main program: diff --git a/applications/test/PtrList/Test-PtrList.C b/applications/test/PtrList/Test-PtrList.C index 115149841f..b10794ffb0 100644 --- a/applications/test/PtrList/Test-PtrList.C +++ b/applications/test/PtrList/Test-PtrList.C @@ -49,15 +49,8 @@ public: static bool verbose; - constexpr Scalar() noexcept - : - data_(0) - {} - - Scalar(scalar val) noexcept - : - data_(val) - {} + constexpr Scalar() noexcept : data_(0) {} + Scalar(scalar val) noexcept : data_(val) {} ~Scalar() { @@ -67,10 +60,7 @@ public: scalar value() const noexcept { return data_; } scalar& value() noexcept { return data_; } - autoPtr clone() const - { - return autoPtr::New(data_); - } + autoPtr clone() const { return autoPtr::New(data_); } friend Ostream& operator<<(Ostream& os, const Scalar& item) { @@ -361,15 +351,19 @@ int main(int argc, char *argv[]) list2.emplace(i, (10 + 1.3*i)); } - #if 0 list2.release(5); list2.release(10); - forAll(list2, i) { - list2.try_emplace(i, (50 + 1.3*i)); + // Memory error (with fulldebug): const label len = (list2.size()+2); + const label len = list2.size(); + Info<< "try_emplace " << len << " values" << nl; + + for (label i = 0; i < len; ++i) + { + list2.try_emplace(i, (50 + 1.3*i)); + } } - #endif PtrList listApp; for (label i = 0; i < 5; ++i) diff --git a/applications/test/PtrListDictionary/Test-PtrListDictionary.C b/applications/test/PtrListDictionary/Test-PtrListDictionary.C index 7f96479dac..28a0020827 100644 --- a/applications/test/PtrListDictionary/Test-PtrListDictionary.C +++ b/applications/test/PtrListDictionary/Test-PtrListDictionary.C @@ -45,28 +45,29 @@ class Scalar public: - Scalar() - : - data_(0) - {} + static bool verbose; - Scalar(scalar val) - : - data_(val) - {} + constexpr Scalar() noexcept : data_(0) {} + Scalar(scalar val) noexcept : data_(val) {} ~Scalar() { - Info<<"delete Scalar: " << data_ << endl; + if (verbose) Info<< "delete Scalar: " << data_ << endl; } - friend Ostream& operator<<(Ostream& os, const Scalar& val) + const scalar& value() const noexcept { return data_; } + scalar& value() noexcept { return data_; } + + autoPtr clone() const { return autoPtr::New(data_); } + + friend Ostream& operator<<(Ostream& os, const Scalar& item) { - os << val.data_; + os << item.value(); return os; } }; +bool Scalar::verbose = true; // * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * // diff --git a/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTable.H b/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTable.H index 3d15ee7087..cb28159c48 100644 --- a/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTable.H +++ b/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTable.H @@ -78,6 +78,14 @@ class HashPtrTable template void read(const dictionary& dict, const INew& inew); + //- Implementation for emplace_set and try_emplace + template + inline T& emplaceImpl + ( + const bool overwrite, + const Key& key, + Args&&... args + ); public: @@ -233,6 +241,14 @@ public: template inline T& emplace_set(const Key& key, Args&&... args); + //- Like emplace_set() but will not overwrite an occupied (non-null) + //- location. + // \param key - the location to set (unless already defined) + // \param args arguments to forward to the constructor of the element + // \return reference to the existing or the new element. + template + inline T& try_emplace(const Key& key, Args&&... args); + //- No insert() with raw pointers (potential memory leaks). //- Use insert() with autoPtr or set() inline bool insert(const Key&, T*) = delete; diff --git a/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTableI.H b/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTableI.H index 8c0a97f4e1..d870d58209 100644 --- a/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTableI.H +++ b/src/OpenFOAM/containers/HashTables/HashPtrTable/HashPtrTableI.H @@ -74,6 +74,42 @@ inline bool Foam::HashPtrTable::emplace } +template +template +inline T& Foam::HashPtrTable::emplaceImpl +( + const bool overwrite, + const Key& key, + Args&&... args +) +{ + T* ptr = nullptr; + + // Replace existing entry unconditionally (overwrite = true) + // or only if nullptr (overwrite = false) + iterator iter(this->find(key)); + if (iter.good()) + { + ptr = iter.val(); + + if (overwrite || !ptr) + { + // Overwrite existing or replace nullptr + delete ptr; + ptr = new T(std::forward(args)...); + iter.val() = ptr; + } + } + else + { + ptr = new T(std::forward(args)...); + this->parent_type::set(key, ptr); + } + + return *ptr; +} + + template template inline T& Foam::HashPtrTable::emplace_set @@ -82,9 +118,21 @@ inline T& Foam::HashPtrTable::emplace_set Args&&... args ) { - T* ptr = new T(std::forward(args)...); - (void)this->set(key, ptr); - return *ptr; + // overwrite = true + return this->emplaceImpl(true, key, std::forward(args)...); +} + + +template +template +inline T& Foam::HashPtrTable::try_emplace +( + const Key& key, + Args&&... args +) +{ + // overwrite = false + return this->emplaceImpl(false, key, std::forward(args)...); } @@ -129,16 +177,20 @@ inline bool Foam::HashPtrTable::set T* ptr ) { - const T* old = this->get(key); - - const bool ok = this->parent_type::set(key, ptr); - - if (ok && old != ptr) + // Replace existing entry unconditionally + iterator iter(this->find(key)); + if (iter.good()) { - delete const_cast(old); + if (iter.val() != ptr) + { + delete iter.val(); + iter.val() = ptr; + } + return true; } - return ok; + // Or add new entry + return this->parent_type::set(key, ptr); } diff --git a/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynList.H b/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynList.H index e89d74d700..365a2f4249 100644 --- a/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynList.H +++ b/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynList.H @@ -190,6 +190,13 @@ public: template inline T& emplace(const label i, Args&&... args); + //- Like emplace_set() but will not overwrite an occupied location. + // \param i - the location to set (unless already defined) + // \param args arguments to forward to the constructor of the element + // \return reference to the existing or the new list element. + template + inline T& try_emplace(const label i, Args&&... args); + //- Set element to given pointer and return old element (can be null). //- Auto-sizes list as required. inline autoPtr set(const label i, T* ptr); diff --git a/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynListI.H b/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynListI.H index 818d2a977a..a94cf0a4c5 100644 --- a/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynListI.H +++ b/src/OpenFOAM/containers/PtrLists/PtrDynList/PtrDynListI.H @@ -399,7 +399,7 @@ inline T& Foam::PtrDynList::emplace_set { resize(i+1); } - return this->PtrList::emplace_set(i, std::forward(args)...); + return PtrList::emplace_set(i, std::forward(args)...); } @@ -415,6 +415,22 @@ inline T& Foam::PtrDynList::emplace } +template +template +inline T& Foam::PtrDynList::try_emplace +( + const label i, + Args&&... args +) +{ + if (i >= this->size()) + { + resize(i+1); + } + return PtrList::try_emplace(i, std::forward(args)...); +} + + template inline Foam::autoPtr Foam::PtrDynList::set ( diff --git a/src/OpenFOAM/containers/PtrLists/PtrList/PtrList.H b/src/OpenFOAM/containers/PtrLists/PtrList/PtrList.H index 1d3cbedfea..5cd1ee389a 100644 --- a/src/OpenFOAM/containers/PtrLists/PtrList/PtrList.H +++ b/src/OpenFOAM/containers/PtrLists/PtrList/PtrList.H @@ -187,6 +187,14 @@ public: template inline T& emplace(const label i, Args&&... args); + //- Like emplace_set() but will not overwrite an occupied (non-null) + //- location. + // \param i - the location to set (unless already defined) + // \param args arguments to forward to the constructor of the element + // \return reference to the existing or the new list element. + template + inline T& try_emplace(const label i, Args&&... args); + //- Set element to given pointer and return old element (can be null) // No-op if the new pointer value is identical to the current content. inline autoPtr set(const label i, T* ptr); diff --git a/src/OpenFOAM/containers/PtrLists/PtrList/PtrListI.H b/src/OpenFOAM/containers/PtrLists/PtrList/PtrListI.H index 6bd161967a..ea982933c5 100644 --- a/src/OpenFOAM/containers/PtrLists/PtrList/PtrListI.H +++ b/src/OpenFOAM/containers/PtrLists/PtrList/PtrListI.H @@ -195,6 +195,19 @@ inline T& Foam::PtrList::emplace(const label i, Args&&... args) } +template +template +inline T& Foam::PtrList::try_emplace(const label i, Args&&... args) +{ + T* ptr = UPtrList::get(i); + if (ptr) + { + return *ptr; + } + return this->emplace_set(i, std::forward(args)...); +} + + template inline Foam::autoPtr Foam::PtrList::set(const label i, T* ptr) {