From 14373d554367d04e64e89a7edb03724b952e9523 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 29 Sep 2025 18:04:56 -0700 Subject: [PATCH] [heap] introduce `Move()` and unify `TakeFrom()` for move semantics (#11972) This change enhances the move semantics for heap-allocated container classes (`Array`, `Data`, and `String`) by introducing a consistent pattern. A new `Move()` method is added to `Heap::Array`, `Heap::Data`, and `Heap::String`. This method returns an rvalue reference to the object, making the intent to transfer ownership explicit at the call site. The existing `TakeFrom()` methods are updated to accept an rvalue reference and now include a check to prevent self-assignment, which improves robustness. For consistency, `Heap::Data::SetFrom(Data&&)` and `Heap::String::Set(String&&)` are renamed to `TakeFrom()`. All call sites are updated to use the new `foo.TakeFrom(bar.Move())` pattern, replacing the more verbose and less clear `static_cast<...&&>(bar)`. Unit tests are updated to validate the new `TakeFrom()` and `Move()` semantics, including tests for self-assignment and moving from a null (empty) container --- src/core/border_router/routing_manager.cpp | 2 +- src/core/common/heap_array.hpp | 34 ++++++++++++----- src/core/common/heap_data.cpp | 16 ++++---- src/core/common/heap_data.hpp | 21 +++++++++-- src/core/common/heap_string.cpp | 16 ++++---- src/core/common/heap_string.hpp | 20 +++++++--- src/core/net/mdns.cpp | 6 +-- src/core/net/srp_advertising_proxy.cpp | 2 +- tests/unit/test_heap_array.cpp | 28 +++++++++++++- tests/unit/test_heap_string.cpp | 44 +++++++++++++++++++++- 10 files changed, 145 insertions(+), 44 deletions(-) diff --git a/src/core/border_router/routing_manager.cpp b/src/core/border_router/routing_manager.cpp index f9a1418df..01b1cd702 100644 --- a/src/core/border_router/routing_manager.cpp +++ b/src/core/border_router/routing_manager.cpp @@ -3708,7 +3708,7 @@ Error RoutingManager::RioAdvertiser::AppendRios(RouterAdvert::TxMessage &aRaMess const OmrPrefixManager &omrPrefixManager = Get().mOmrPrefixManager; #if OPENTHREAD_CONFIG_BORDER_ROUTING_USE_HEAP_ENABLE - oldPrefixes.TakeFrom(static_cast(mPrefixes)); + oldPrefixes.TakeFrom(mPrefixes.Move()); #else oldPrefixes = mPrefixes; #endif diff --git a/src/core/common/heap_array.hpp b/src/core/common/heap_array.hpp index d9982477e..ea835acbc 100644 --- a/src/core/common/heap_array.hpp +++ b/src/core/common/heap_array.hpp @@ -153,21 +153,37 @@ public: Error ReserveCapacity(IndexType aCapacity) { return Allocate(aCapacity); } /** - * Sets the array by taking the buffer from another given array (using move semantics). + * Sets the array by taking ownership of the buffer from another `Array`. * - * @param[in] aOther The other `Heap::Array` to take from (rvalue reference). + * This method uses move semantics. After the call, `aOther` will be empty and this `Array` will hold the + * buffer previously held by `aOther`. + * + * @param[in] aOther An rvalue reference to another `Array` to take from. */ void TakeFrom(Array &&aOther) { - Free(); - mArray = aOther.mArray; - mLength = aOther.mLength; - mCapacity = aOther.mCapacity; - aOther.mArray = nullptr; - aOther.mLength = 0; - aOther.mCapacity = 0; + if (&aOther != this) + { + Free(); + mArray = aOther.mArray; + mLength = aOther.mLength; + mCapacity = aOther.mCapacity; + aOther.mArray = nullptr; + aOther.mLength = 0; + aOther.mCapacity = 0; + } } + /** + * Casts the `Array` to an rvalue reference. + * + * This method is intended to be used with `TakeFrom()` to explicitly indicate a move operation and transfer of + * the underlying buffer. + * + * @returns An rvalue reference to this `Array`. + */ + Array &&Move(void) { return static_cast(*this); } + /** * Overloads the `[]` operator to get the element at a given index. * diff --git a/src/core/common/heap_data.cpp b/src/core/common/heap_data.cpp index 9c09e5903..1da083817 100644 --- a/src/core/common/heap_data.cpp +++ b/src/core/common/heap_data.cpp @@ -72,10 +72,14 @@ exit: return error; } -void Data::SetFrom(Data &&aData) +void Data::TakeFrom(Data &&aData) { - Free(); - TakeFrom(aData); + if (&aData != this) + { + Free(); + mData.Init(aData.mData.GetBytes(), aData.GetLength()); + aData.mData.Init(nullptr, 0); + } } bool Data::Matches(const uint8_t *aBuffer, uint16_t aLength) const @@ -127,11 +131,5 @@ exit: return error; } -void Data::TakeFrom(Data &aData) -{ - mData.Init(aData.mData.GetBytes(), aData.GetLength()); - aData.mData.Init(nullptr, 0); -} - } // namespace Heap } // namespace ot diff --git a/src/core/common/heap_data.hpp b/src/core/common/heap_data.hpp index 0b51ce98e..ec6cc859d 100644 --- a/src/core/common/heap_data.hpp +++ b/src/core/common/heap_data.hpp @@ -62,7 +62,7 @@ public: * * @param[in] aData An rvalue reference to another `Heap::Data` to move from. */ - Data(Data &&aData) { TakeFrom(aData); } + Data(Data &&aData) { TakeFrom(aData.Move()); } /** * This is the destructor for `Heap::Data` object. @@ -131,11 +131,24 @@ public: Error SetFrom(const Message &aMessage, uint16_t aOffset, uint16_t aLength); /** - * Sets the `Heap::Data` from another one (move semantics). + * Sets the `Heap::Data` by taking ownership of the buffer from another `Heap::Data`. * - * @param[in] aData The other `Heap::Data` to set from (rvalue reference). + * This method uses move semantics. After the call, `aData` will be null and this `Data` will hold the buffer + * previously held by `aData`. + * + * @param[in] aData An rvalue reference to another `Heap::Data` to take from. */ - void SetFrom(Data &&aData); + void TakeFrom(Data &&aData); + + /** + * Casts the `Heap::Data` to an rvalue reference. + * + * This method is intended to be used with `TakeFrom()` to explicitly indicate a move operation and transfer of + * the underlying buffer. + * + * @returns An rvalue reference to this `Heap::Data`. + */ + Data &&Move(void) { return static_cast(*this); } /** * Appends the bytes from `Heap::Data` to a given message. diff --git a/src/core/common/heap_string.cpp b/src/core/common/heap_string.cpp index b286b3268..12ad6b88a 100644 --- a/src/core/common/heap_string.cpp +++ b/src/core/common/heap_string.cpp @@ -66,16 +66,14 @@ exit: return error; } -Error String::Set(String &&aString) +void String::TakeFrom(String &&aString) { - VerifyOrExit(mStringBuffer != aString.mStringBuffer); - - Heap::Free(mStringBuffer); - mStringBuffer = aString.mStringBuffer; - aString.mStringBuffer = nullptr; - -exit: - return kErrorNone; + if (&aString != this) + { + Heap::Free(mStringBuffer); + mStringBuffer = aString.mStringBuffer; + aString.mStringBuffer = nullptr; + } } void String::Free(void) diff --git a/src/core/common/heap_string.hpp b/src/core/common/heap_string.hpp index b576194de..d840e9c58 100644 --- a/src/core/common/heap_string.hpp +++ b/src/core/common/heap_string.hpp @@ -115,14 +115,24 @@ public: Error Set(const String &aString) { return Set(aString.AsCString()); } /** - * Sets the string from another `String`. + * Sets the string by taking ownership of the buffer from another `String`. * - * @param[in] aString The other `String` to set from (rvalue reference using move semantics). + * This method uses move semantics. After the call, `aString` will be null and this `String` will hold the + * buffer previously held by `aString`. * - * @retval kErrorNone Successfully set the string. - * @retval kErrorNoBufs Failed to allocate buffer for string. + * @param[in] aString An rvalue reference to another `String` to take from. */ - Error Set(String &&aString); + void TakeFrom(String &&aString); + + /** + * Casts the `String` to an rvalue reference. + * + * This method is intended to be used with `TakeFrom()` to explicitly indicate a move operation and transfer of + * the underlying buffer. + * + * @returns An rvalue reference to this `String`. + */ + String &&Move(void) { return static_cast(*this); } /** * Frees any buffer allocated by the `String`. diff --git a/src/core/net/mdns.cpp b/src/core/net/mdns.cpp index fc8e5eb6f..f83e9ba74 100644 --- a/src/core/net/mdns.cpp +++ b/src/core/net/mdns.cpp @@ -1727,7 +1727,7 @@ void Core::LocalHost::HandleEventTimer(void) AddressArray &addresses = (addrType == kIp4AddrType) ? mIp4Addresses : mIp6Addresses; AddressArray oldAddresses; - oldAddresses.TakeFrom(static_cast(addresses)); + oldAddresses.TakeFrom(addresses.Move()); addresses.Clear(); // First, add existing addresses (from old list) that did not @@ -7450,7 +7450,7 @@ Core::RecordCache::NewRecordEntry::NewRecordEntry(const ResourceRecord &aRecord, , mCacheFlush(aRecord.GetClass() & kClassCacheFlushFlag) , mType(aRecord.GetType()) , mTtl(aRecord.GetTtl()) - , mData(static_cast(aData)) + , mData(aData.Move()) { } @@ -7467,7 +7467,7 @@ bool Core::RecordCache::NewRecordEntry::Matches(uint16_t aType, const Heap::Data Core::RecordCache::RecordEntry::RecordEntry(NewRecordEntry &aNewEntry) : mNext(nullptr) , mType(aNewEntry.mType) - , mData(static_cast(aNewEntry.mData)) + , mData(aNewEntry.mData.Move()) { mRecord.RefreshTtl(aNewEntry.mTtl); } diff --git a/src/core/net/srp_advertising_proxy.cpp b/src/core/net/srp_advertising_proxy.cpp index a2c55c69c..207f8d863 100644 --- a/src/core/net/srp_advertising_proxy.cpp +++ b/src/core/net/srp_advertising_proxy.cpp @@ -1006,7 +1006,7 @@ void AdvertisingProxy::RegisterService(Service &aService) IgnoreError(Server::Service::ParseSubTypeServiceName(subTypeName.AsCString(), label, sizeof(label))); SuccessOrExit(error = labelString.Set(label)); - IgnoreError(subTypeHeapStrings.PushBack(static_cast(labelString))); + IgnoreError(subTypeHeapStrings.PushBack(labelString.Move())); IgnoreError(subTypeLabels.PushBack(subTypeHeapStrings.Back()->AsCString())); } diff --git a/tests/unit/test_heap_array.cpp b/tests/unit/test_heap_array.cpp index bb95185ca..144611da7 100644 --- a/tests/unit/test_heap_array.cpp +++ b/tests/unit/test_heap_array.cpp @@ -326,7 +326,7 @@ void TestHeapArrayOfUint16(void) VerifyArray(array2, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26); - array2.TakeFrom(static_cast &&>(array)); + array2.TakeFrom(array.Move()); VerifyArray(array); VerifyOrQuit(array.GetCapacity() == 0); @@ -334,6 +334,18 @@ void TestHeapArrayOfUint16(void) VerifyArray(array2, 0, 1, 2, 3, 4, 5); VerifyOrQuit(array2.GetCapacity() == 10); + // Test moving from self + array2.TakeFrom(array2.Move()); + VerifyArray(array2, 0, 1, 2, 3, 4, 5); + VerifyOrQuit(array2.GetCapacity() == 10); + + // Test moving a null array + array2.TakeFrom(array.Move()); + VerifyArray(array); + VerifyOrQuit(array.GetCapacity() == 0); + VerifyArray(array2); + VerifyOrQuit(array2.GetCapacity() == 0); + printf("\n -- PASS\n"); } @@ -465,13 +477,25 @@ void TestHeapArray(void) SuccessOrQuit(array2.PushBack(Entry(num + 0x20))); } - array2.TakeFrom(static_cast &&>(array)); + array2.TakeFrom(array.Move()); VerifyOrQuit(array.GetLength() == 0); VerifyOrQuit(array.GetCapacity() == 0); VerifyArray(array2, 0, 1, 2, 3, 4, 5); VerifyOrQuit(array2.GetCapacity() == 10); + + // Test moving from self + array2.TakeFrom(array2.Move()); + VerifyArray(array2, 0, 1, 2, 3, 4, 5); + VerifyOrQuit(array2.GetCapacity() == 10); + + // Test moving a null array + array2.TakeFrom(array.Move()); + VerifyArray(array); + VerifyOrQuit(array.GetCapacity() == 0); + VerifyArray(array2); + VerifyOrQuit(array2.GetCapacity() == 0); } printf("------------------------------------------------------------------------------------\n"); diff --git a/tests/unit/test_heap_string.cpp b/tests/unit/test_heap_string.cpp index 3feb4b65f..a4659b682 100644 --- a/tests/unit/test_heap_string.cpp +++ b/tests/unit/test_heap_string.cpp @@ -139,6 +139,27 @@ void TestHeapString(void) SuccessOrQuit(str1.Set(GetName()), "Set() with move semantics failed"); VerifyString("str1", str1, "name"); + printf("------------------------------------------------------------------------------------\n"); + printf("TakeFrom() and Move()\n\n"); + + SuccessOrQuit(str1.Set("string to be moved")); + str2.Free(); + VerifyString("str1", str1, "string to be moved"); + VerifyString("str2", str2, nullptr); + + str2.TakeFrom(str1.Move()); + VerifyString("str1", str1, nullptr); + VerifyString("str2", str2, "string to be moved"); + + // Test moving from self + str2.TakeFrom(str2.Move()); + VerifyString("str2", str2, "string to be moved"); + + // Test moving a null string + str2.TakeFrom(str1.Move()); + VerifyString("str1", str1, nullptr); + VerifyString("str2", str2, nullptr); + printf("------------------------------------------------------------------------------------\n"); printf("operator==() with two null string\n\n"); str1.Free(); @@ -208,6 +229,7 @@ void TestHeapData(void) MessagePool *messagePool; Message *message; Heap::Data data; + Heap::Data data2; uint16_t offset; const uint8_t *oldBuffer; @@ -314,9 +336,29 @@ void TestHeapData(void) printf("------------------------------------------------------------------------------------\n"); printf("SetFrom() move semantics\n\n"); - data.SetFrom(GetData()); + data.TakeFrom(GetData().Move()); VerifyData(data, &kTestValue, sizeof(kTestValue)); + printf("------------------------------------------------------------------------------------\n"); + printf("TakeFrom() and Move()\n\n"); + + SuccessOrQuit(data.SetFrom(kData1, sizeof(kData1))); + VerifyData(data, kData1); + VerifyData(data2, nullptr, 0); + + data2.TakeFrom(data.Move()); + VerifyData(data, nullptr, 0); + VerifyData(data2, kData1); + + // Test moving from self + data2.TakeFrom(data2.Move()); + VerifyData(data2, kData1); + + // Test moving a null data + data2.TakeFrom(data.Move()); + VerifyData(data, nullptr, 0); + VerifyData(data2, nullptr, 0); + printf("\n -- PASS\n"); }