From 9c33b948e95b71b5da3ad14f6ea71dc842abbae9 Mon Sep 17 00:00:00 2001 From: Adeel Kazmi Date: Fri, 19 Oct 2018 12:46:35 +0100 Subject: [PATCH] Make OwnerPointer Movable & add test cases Change-Id: If01e25003536ff26ed67a620d094f2311a8e0340 --- automated-tests/src/dali-internal/CMakeLists.txt | 5 +- .../utc-Dali-Internal-OwnerPointer.cpp | 361 +++++++++++++++++++++ dali/internal/common/owner-pointer.h | 68 ++-- 3 files changed, 409 insertions(+), 25 deletions(-) create mode 100644 automated-tests/src/dali-internal/utc-Dali-Internal-OwnerPointer.cpp diff --git a/automated-tests/src/dali-internal/CMakeLists.txt b/automated-tests/src/dali-internal/CMakeLists.txt index 5fc4d99..64b34e7 100644 --- a/automated-tests/src/dali-internal/CMakeLists.txt +++ b/automated-tests/src/dali-internal/CMakeLists.txt @@ -7,10 +7,11 @@ SET(CAPI_LIB "dali-internal") SET(TC_SOURCES utc-Dali-Internal-Core.cpp - utc-Dali-Internal-Handles.cpp utc-Dali-Internal-FixedSizeMemoryPool.cpp - utc-Dali-Internal-MemoryPoolObjectAllocator.cpp utc-Dali-Internal-FrustumCulling.cpp + utc-Dali-Internal-Handles.cpp + utc-Dali-Internal-MemoryPoolObjectAllocator.cpp + utc-Dali-Internal-OwnerPointer.cpp ) LIST(APPEND TC_SOURCES diff --git a/automated-tests/src/dali-internal/utc-Dali-Internal-OwnerPointer.cpp b/automated-tests/src/dali-internal/utc-Dali-Internal-OwnerPointer.cpp new file mode 100644 index 0000000..1452895 --- /dev/null +++ b/automated-tests/src/dali-internal/utc-Dali-Internal-OwnerPointer.cpp @@ -0,0 +1,361 @@ +/* + * Copyright (c) 2018 Samsung Electronics Co., Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +// EXTERNAL INCLUDES +#include + +// INTERNAL INCLUDES +#include +#include + +using namespace Dali::Internal; + +void utc_dali_internal_owner_pointer_startup(void) +{ + test_return_value = TET_UNDEF; +} + +void utc_dali_internal_owner_pointer_cleanup(void) +{ + test_return_value = TET_PASS; +} + +/////////////////////////////////////////////////////////////////////////////// +namespace +{ + +/// Takes in a reference to a bool which is set to true when the destructor is called +class OwnedClass +{ +public: + + OwnedClass( bool& destructorCalled ) + : mDestructorCalled( destructorCalled ) + { + mDestructorCalled = false; + } + + ~OwnedClass() + { + mDestructorCalled = true; + } + +private: + + bool& mDestructorCalled; +}; + +/// Just a simple class with a function that marks a member boolean to true if that function is called +class ClassWithFunction +{ +public: + ClassWithFunction() = default; + + void MyFunction() + { + functionCalled = true; + } + + bool functionCalled{ false }; +}; + +} // anon namespace + +/////////////////////////////////////////////////////////////////////////////// + +int UtcDaliOwnerPointerEnsureDeletion(void) +{ + // Ensure that the object owned by the owner-pointer is deleted. + + bool deleted = false; + + { + OwnerPointer< OwnedClass > pointer( new OwnedClass( deleted ) ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + } + + // OwnerPointer out-of-scope, object should be deleted. + + DALI_TEST_EQUALS( deleted, true, TEST_LOCATION ); + + END_TEST; +} + +int UtcDaliOwnerPointerDefaultConstructor(void) +{ + // Ensure the default constructor is created as expected. + + OwnerPointer< OwnedClass > pointer; + DALI_TEST_CHECK( pointer.Get() == nullptr ); + + END_TEST; +} + +int UtcDaliOwnerPointerCopy(void) +{ + // Call copy constructor and assignment operator + + bool deleted = false; + OwnedClass* owned = new OwnedClass( deleted ); + + OwnerPointer< OwnedClass > first( owned ); + DALI_TEST_CHECK( first.Get() == owned ); + + { + // Copy constructor, first should have a nullptr now, no object deletion + OwnerPointer< OwnedClass > second( first ); + DALI_TEST_CHECK( first.Get() == nullptr ); + DALI_TEST_CHECK( second.Get() == owned); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + + // Self assignment, nothing should change or be deleted. + first = first; + second = second; + DALI_TEST_CHECK( first.Get() == nullptr ); + DALI_TEST_CHECK( second.Get() == owned ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + + // Assign second to first, no deletion, second should have a nullptr now + first = second; + DALI_TEST_CHECK( first.Get() == owned ); + DALI_TEST_CHECK( second.Get() == nullptr ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + } + + // second is out-of-scope now, no object deletion + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + + // Assign to an empty pointer, owned object should be deleted + OwnerPointer< OwnedClass > empty; + first = empty; + DALI_TEST_EQUALS( deleted, true, TEST_LOCATION ); + DALI_TEST_CHECK( first.Get() == nullptr ); + DALI_TEST_CHECK( empty.Get() == nullptr ); + + END_TEST; +} + +int UtcDaliOwnerPointerMove(void) +{ + // Call move constructor and move assignment operator + + bool deleted = false; + OwnedClass* owned = new OwnedClass( deleted ); + + OwnerPointer< OwnedClass > first( owned ); + DALI_TEST_CHECK( first.Get() == owned ); + + { + // Move constructor, first should have a nullptr now, no object deletion + OwnerPointer< OwnedClass > second( std::move( first ) ); + DALI_TEST_CHECK( first.Get() == nullptr ); + DALI_TEST_CHECK( second.Get() == owned); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + + // Self assignment, nothing should change or be deleted. + first = std::move( first ); + second = std::move( second ); + DALI_TEST_CHECK( first.Get() == nullptr ); + DALI_TEST_CHECK( second.Get() == owned ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + + // Assign second to first, no deletion, second should have a nullptr now + first = std::move( second ); + DALI_TEST_CHECK( first.Get() == owned ); + DALI_TEST_CHECK( second.Get() == nullptr ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + } + + // second is out-of-scope now, no object deletion + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + + // Assign to an empty pointer, owned object should be deleted + OwnerPointer< OwnedClass > empty; + first = std::move( empty ); + DALI_TEST_EQUALS( deleted, true, TEST_LOCATION ); + DALI_TEST_CHECK( first.Get() == nullptr ); + DALI_TEST_CHECK( empty.Get() == nullptr ); + + END_TEST; +} + +int UtcDaliOwnerPointerIndirection(void) +{ + // Check the indirection operators + + using Ptr = OwnerPointer< int >; + + { + int* rawIntPtr( new int( 200 ) ); + Ptr nonConstPtr( rawIntPtr ); + DALI_TEST_CHECK( rawIntPtr == &( *nonConstPtr ) ); + DALI_TEST_EQUALS( *nonConstPtr, 200, TEST_LOCATION ); + } + + { + int* rawIntPtr2( new int( 300 ) ); + const Ptr constPtr( rawIntPtr2 ); + DALI_TEST_CHECK( rawIntPtr2 == &( *constPtr ) ); + DALI_TEST_EQUALS( *constPtr, 300, TEST_LOCATION ); + } + + END_TEST; +} + +int UtcDaliOwnerPointerPointerOperator(void) +{ + // Check the pointer operators + + using Ptr = OwnerPointer< ClassWithFunction >; + + // Check if function is called as expected when using a const OwnerPointer + { + ClassWithFunction* rawPtr( new ClassWithFunction ); + Ptr nonConstPtr( rawPtr ); + DALI_TEST_EQUALS( rawPtr->functionCalled, false, TEST_LOCATION ); + nonConstPtr->MyFunction(); + DALI_TEST_EQUALS( rawPtr->functionCalled, true, TEST_LOCATION ); + } + + + // Check if function is called as expected when using a const OwnerPointer + { + ClassWithFunction* rawPtr2( new ClassWithFunction ); + const Ptr constPtr( rawPtr2 ); + DALI_TEST_EQUALS( rawPtr2->functionCalled, false, TEST_LOCATION ); + constPtr->MyFunction(); + DALI_TEST_EQUALS( rawPtr2->functionCalled, true, TEST_LOCATION ); + } + END_TEST; +} + +int UtcDaliOwnerPointerComparisonOperator(void) +{ + // Check the comparison operator + + using Ptr = OwnerPointer< int >; + + int* rawIntPtr( new int( 200 ) ); + Ptr ownerPtr( rawIntPtr ); + DALI_TEST_CHECK( ownerPtr == rawIntPtr ); + DALI_TEST_CHECK( ! ( ownerPtr == nullptr ) ); + + END_TEST; +} + +int UtcDaliOwnerPointerReset(void) +{ + // Ensure that calling Reset deletes the object and sets the owner-pointer to NULL + + bool deleted = false; + + OwnerPointer< OwnedClass > pointer( new OwnedClass( deleted ) ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + pointer.Reset(); + DALI_TEST_EQUALS( deleted, true, TEST_LOCATION ); + DALI_TEST_CHECK( pointer.Get() == nullptr ); + + // Reset the empty pointer, should have no effect but there shouldn't be any crash + pointer.Reset(); + DALI_TEST_CHECK( pointer.Get() == nullptr ); + + END_TEST; +} + +int UtcDaliOwnerPointerRelease(void) +{ + // Ensure that calling Release does NOT delete the object but still sets the owner-pointer to NULL + + bool deleted = false; + + OwnedClass* rawPtr = new OwnedClass( deleted ); + OwnerPointer< OwnedClass > pointer( rawPtr ); + + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( pointer.Release() == rawPtr ); + DALI_TEST_EQUALS( deleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( pointer.Get() == nullptr ); + + // Release the empty pointer, should have no effect but there shouldn't be any crash + DALI_TEST_CHECK( pointer.Release() == nullptr ); + DALI_TEST_CHECK( pointer.Get() == nullptr ); + + END_TEST; +} + +int UtcDaliOwnerPointerGet(void) +{ + // Check the Get method + + using Ptr = OwnerPointer< int >; + + int* rawIntPtr( new int( 200 ) ); + Ptr ownerPtr( rawIntPtr ); + DALI_TEST_CHECK( ownerPtr.Get() == rawIntPtr ); + + END_TEST; +} + +int UtcDaliOwnerPointerSwap(void) +{ + // Ensure the Swap method swaps the pointers and doesn't delete any objects + + using Ptr = OwnerPointer< OwnedClass >; + + bool firstObjectDeleted = false; + bool secondObjectDeleted = false; + + OwnedClass* firstRawPtr = new OwnedClass( firstObjectDeleted ); + OwnedClass* secondRawPtr = new OwnedClass( secondObjectDeleted ); + + Ptr firstPtr( firstRawPtr ); + Ptr secondPtr( secondRawPtr ); + + // Check initial values + DALI_TEST_EQUALS( firstObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_EQUALS( secondObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( firstPtr == firstRawPtr ); + DALI_TEST_CHECK( secondPtr == secondRawPtr ); + + // Call Swap on first and ensure swap is done and there's no deletion + firstPtr.Swap( secondPtr ); + DALI_TEST_EQUALS( firstObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_EQUALS( secondObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( firstPtr == secondRawPtr ); + DALI_TEST_CHECK( secondPtr == firstRawPtr ); + + // Swap back using second, again no deletion + secondPtr.Swap( firstPtr ); + DALI_TEST_EQUALS( firstObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_EQUALS( secondObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( firstPtr == firstRawPtr ); + DALI_TEST_CHECK( secondPtr == secondRawPtr ); + + // Swap with self, nothing should change or be deleted + firstPtr.Swap( firstPtr ); + DALI_TEST_EQUALS( firstObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( firstPtr == firstRawPtr ); + + // Swap with an empty OwnerPointer, no deletion but firstPtr should be empty now + Ptr emptyPtr; + firstPtr.Swap( emptyPtr ); + DALI_TEST_EQUALS( firstObjectDeleted, false, TEST_LOCATION ); + DALI_TEST_CHECK( firstPtr == nullptr ); + DALI_TEST_CHECK( emptyPtr == firstRawPtr ); + + END_TEST; +} diff --git a/dali/internal/common/owner-pointer.h b/dali/internal/common/owner-pointer.h index e780920..165747a 100644 --- a/dali/internal/common/owner-pointer.h +++ b/dali/internal/common/owner-pointer.h @@ -2,7 +2,7 @@ #define __DALI_INTERNAL_OWNER_POINTER_H__ /* - * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * Copyright (c) 2018 Samsung Electronics Co., Ltd. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,9 +18,6 @@ * */ -// EXTERNAL INCLUDES -#include // NULL - // INTERNAL INCLUDES #include @@ -30,15 +27,19 @@ namespace Dali namespace Internal { -template < typename T > +template< typename T > class OwnerPointer { public: + /** * Default constructor. Creates an OwnerPointer that does not own any object. + * @note This does not protect against two different OwnerPointers pointing to the same object. + * Both OwnerPointers will try to release the memory of the same object in that case which + * could lead to a crash. */ OwnerPointer() - : mObject(NULL) + : mObject( nullptr ) { } @@ -47,7 +48,7 @@ public: * @param[in] object A pointer to a heap allocated object. */ OwnerPointer( T* object ) - : mObject(object) + : mObject( object ) { } @@ -56,11 +57,20 @@ public: * @param[in] other The pointer that gives away the ownership. */ OwnerPointer( const OwnerPointer& other ) - : mObject(NULL) + : OwnerPointer( static_cast< OwnerPointer&& >( const_cast( other ) ) ) // Remove constness & cast to rvalue to use the move constructor { // other needs to be const for compiler to pick up this as copy constructor; // though we are using this as move as there can only be one owner - Swap( const_cast( other ) ); + } + + /** + * Move constructor. Passes the ownership of a pointer to another. + * @param[in] other The pointer that gives away the ownership. + */ + OwnerPointer( OwnerPointer&& other ) + : mObject( nullptr ) + { + Swap( other ); } /** @@ -71,8 +81,9 @@ public: { if( this != &other ) // no self-assignment { - // Creation of temportaty object to prevent premature deletion of object - OwnerPointer(other).Swap(*this); + delete mObject; + mObject = other.mObject; + other.mObject = nullptr; } // return self @@ -80,6 +91,16 @@ public: } /** + * Move assignment operator. Passes the ownership of a pointer to another. + * @param[in] other The pointer that gives away the ownership. + */ + OwnerPointer& operator=( OwnerPointer&& other ) + { + // Reuse operator= + return operator=( other ); + } + + /** * Assignment operator. Takes the ownership of the object. * If it owns an object already, it will be deleted. * @param[in] pointer A pointer to a heap allocated object. @@ -109,7 +130,7 @@ public: */ T& operator*() { - DALI_ASSERT_DEBUG( mObject != NULL ); + DALI_ASSERT_DEBUG( mObject ); return *mObject; } @@ -120,7 +141,7 @@ public: */ T& operator*() const { - DALI_ASSERT_DEBUG( mObject != NULL ); + DALI_ASSERT_DEBUG( mObject ); // Pointer semantics: A const pointer does not mean const data. return const_cast< T& >( *mObject ); @@ -159,11 +180,8 @@ public: */ void Reset() { - if ( mObject != NULL ) - { - delete mObject; - mObject = NULL; - } + delete mObject; + mObject = nullptr; } /** @@ -173,7 +191,7 @@ public: T* Release() { T* tmp = mObject; - mObject = NULL; + mObject = nullptr; return tmp; } @@ -188,12 +206,16 @@ public: /** * Swap owned objects + * @param[in] other The pointer to swap the owned objects with. */ void Swap( OwnerPointer& other ) { - T* tmp = mObject; - mObject = other.mObject; - other.mObject = tmp; + if( this != &other ) + { + T* tmp = mObject; + mObject = other.mObject; + other.mObject = tmp; + } } // Handle comparisons - This is a variation of the safe bool idiom @@ -209,7 +231,7 @@ public: */ operator BooleanType() const { - return (mObject != NULL) ? &OwnerPointer::ThisIsSaferThanReturningVoidStar : NULL; + return ( mObject != nullptr ) ? &OwnerPointer::ThisIsSaferThanReturningVoidStar : nullptr; } private: -- 2.7.4