From 806176778b6f217e4c5a302e61e85546846d7291 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Artur=20=C5=9Awigo=C5=84?= Date: Thu, 13 Jan 2022 15:23:16 +0100 Subject: [PATCH] Replace obsolete safe bool idiom with explicit operator bool C++11 introduces 'explicit operator bool' to prevent unintended implicit conversions to 'bool', thus making the trick with converting to a pointer-to-member (a.k.a. "safe bool idiom") obsolete. The explicit operator is more restrictive than 'safe bool', and it helped uncover a bug in the test suite where object handles were implicitly converted to bool before being sent to an std::ostream. Change-Id: I8c9d33812bc022570f5c286d925914dba508185c --- .../dali-test-suite-utils.cpp | 5 ++++ .../dali-test-suite-utils/dali-test-suite-utils.h | 1 + .../test-graphics-controller.cpp | 2 +- automated-tests/src/dali/utc-Dali-BaseHandle.cpp | 23 ++---------------- automated-tests/src/dali/utc-Dali-IntrusivePtr.cpp | 18 ++++++-------- dali/internal/common/owner-pointer.h | 20 ++++------------ dali/internal/event/rendering/texture-impl.cpp | 4 ++-- dali/internal/render/renderers/render-texture.cpp | 2 +- dali/internal/render/renderers/render-texture.h | 2 +- .../render-tasks/scene-graph-render-task-list.cpp | 2 +- dali/public-api/common/intrusive-ptr.h | 23 ++++-------------- dali/public-api/object/base-handle.cpp | 4 ++-- dali/public-api/object/base-handle.h | 28 +++------------------- 13 files changed, 34 insertions(+), 100 deletions(-) diff --git a/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.cpp b/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.cpp index 523bd8c..299a0e1 100644 --- a/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.cpp +++ b/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.cpp @@ -78,6 +78,11 @@ std::ostream& operator<<(std::ostream& ostream, Degree angle) return ostream; } +std::ostream& operator<<(std::ostream& ostream, BaseHandle handle) +{ + return ostream << static_cast(handle.GetObjectPtr()); +} + void DALI_TEST_EQUALS(const BaseHandle& baseHandle1, const BaseHandle& baseHandle2, const char* location) { DALI_TEST_EQUALS(baseHandle1, baseHandle2, location); diff --git a/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.h b/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.h index c6e63e7..a070298 100644 --- a/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.h +++ b/automated-tests/src/dali/dali-test-suite-utils/dali-test-suite-utils.h @@ -99,6 +99,7 @@ bool operator==(TimePeriod a, TimePeriod b); std::ostream& operator<<(std::ostream& ostream, TimePeriod value); std::ostream& operator<<(std::ostream& ostream, Radian angle); std::ostream& operator<<(std::ostream& ostream, Degree angle); +std::ostream& operator<<(std::ostream& ostream, BaseHandle handle); /** * Test whether two values are equal. diff --git a/automated-tests/src/dali/dali-test-suite-utils/test-graphics-controller.cpp b/automated-tests/src/dali/dali-test-suite-utils/test-graphics-controller.cpp index d2d8b16..fb2afda 100644 --- a/automated-tests/src/dali/dali-test-suite-utils/test-graphics-controller.cpp +++ b/automated-tests/src/dali/dali-test-suite-utils/test-graphics-controller.cpp @@ -78,7 +78,7 @@ std::ostream& operator<<(std::ostream& o, const Graphics::TextureCreateInfo& cre << " usageFlags:" << std::hex << createInfo.usageFlags << " data:" << std::hex << createInfo.data << " dataSize:" << std::dec << createInfo.dataSize - << " nativeImagePtr:" << std::hex << createInfo.nativeImagePtr; + << " nativeImagePtr:" << std::hex << createInfo.nativeImagePtr.Get(); return o; } diff --git a/automated-tests/src/dali/utc-Dali-BaseHandle.cpp b/automated-tests/src/dali/utc-Dali-BaseHandle.cpp index 5b3d622..b55478a 100644 --- a/automated-tests/src/dali/utc-Dali-BaseHandle.cpp +++ b/automated-tests/src/dali/utc-Dali-BaseHandle.cpp @@ -98,15 +98,6 @@ struct TestCallback class FakeObject : public BaseObject { }; -// used for testing ThisIsSaferThanReturningVoidStar -class FakeHandle : public BaseHandle -{ -public: - void RunTest() - { - return ThisIsSaferThanReturningVoidStar(); - } -}; } // namespace int UtcDaliBaseHandleConstructorVoid(void) @@ -543,16 +534,6 @@ int UtcDaliBaseHandleGetTypeInfoP(void) END_TEST; } -int UtcDaliBaseHandleThisIsSaferThanReturningVoidStar(void) -{ - TestApplication application; - tet_infoline("Testing Dali::BaseHandle::GetTypeInfo"); - FakeHandle handle; - handle.RunTest(); - tet_result(TET_PASS); - END_TEST; -} - int UtcDaliBaseHandleGetTypeInfoN(void) { TestApplication application; @@ -583,12 +564,12 @@ int UtcDaliBaseHandleGetObjectPtr(void) int UtcDaliBaseHandleBooleanCast(void) { TestApplication application; - tet_infoline("Testing Dali::BaseHandle::BooleanType"); + tet_infoline("Testing Dali::BaseHandle::operator bool"); // get the root layer BaseHandle handle = Actor::New(); - DALI_TEST_CHECK(static_cast(handle)); + DALI_TEST_CHECK(static_cast(handle)); END_TEST; } diff --git a/automated-tests/src/dali/utc-Dali-IntrusivePtr.cpp b/automated-tests/src/dali/utc-Dali-IntrusivePtr.cpp index 238b200..77454af 100644 --- a/automated-tests/src/dali/utc-Dali-IntrusivePtr.cpp +++ b/automated-tests/src/dali/utc-Dali-IntrusivePtr.cpp @@ -363,30 +363,26 @@ int UtcDaliIntrusivePtrResetTN(void) END_TEST; } -int UtcDaliIntrusivePtrOperatorBooleanTypeP(void) +int UtcDaliIntrusivePtrOperatorBoolP(void) { - tet_infoline("Positive Test for Dali::IntrusivePtr::operator Booleantype()"); + tet_infoline("Positive Test for Dali::IntrusivePtr::operator bool()"); IntrusivePtr counted(new Counted); - DALI_TEST_CHECK(counted.operator BooleanType() != 0); + DALI_TEST_CHECK(counted.operator bool() == true); DALI_TEST_CHECK(counted); - typedef void (IntrusivePtr::*BoolIdiomFunc)() const; - BoolIdiomFunc func = static_cast(counted.operator BooleanType()); - ((counted).*func)(); // purely for test coverage - counted.Reset(); - DALI_TEST_CHECK(counted.operator BooleanType() == 0); + DALI_TEST_CHECK(counted.operator bool() == false); END_TEST; } -int UtcDaliIntrusivePtrOperatorBooleanTypeN(void) +int UtcDaliIntrusivePtrOperatorBoolN(void) { - tet_infoline("Negative Test for Dali::IntrusivePtr::operator Booleantype()"); + tet_infoline("Negative Test for Dali::IntrusivePtr::operator bool()"); IntrusivePtr counted; - DALI_TEST_CHECK(counted.operator BooleanType() == 0); + DALI_TEST_CHECK(counted.operator bool() == false); DALI_TEST_CHECK(!counted); END_TEST; } diff --git a/dali/internal/common/owner-pointer.h b/dali/internal/common/owner-pointer.h index c50e589..28fe69e 100644 --- a/dali/internal/common/owner-pointer.h +++ b/dali/internal/common/owner-pointer.h @@ -215,30 +215,18 @@ public: } } - // Handle comparisons - This is a variation of the safe bool idiom + // Handle comparisons /** - * Pointer-to-member type. Objects can be implicitly converted to this for validity checks. - */ - using BooleanType = void (OwnerPointer::*)() const; - - /** - * Converts an object handle to a BooleanType. + * Converts an object handle to a bool. * This is useful for checking whether the handle is NULL. */ - operator BooleanType() const + explicit operator bool() const { - return (mObject != nullptr) ? &OwnerPointer::ThisIsSaferThanReturningVoidStar : nullptr; + return mObject != nullptr; } private: - /** - * Used by the safe bool idiom. - */ - void ThisIsSaferThanReturningVoidStar() const - { - } - // data T* mObject; ///< Raw pointer to the object }; diff --git a/dali/internal/event/rendering/texture-impl.cpp b/dali/internal/event/rendering/texture-impl.cpp index 3ddf52b..234b934 100644 --- a/dali/internal/event/rendering/texture-impl.cpp +++ b/dali/internal/event/rendering/texture-impl.cpp @@ -197,13 +197,13 @@ unsigned int Texture::GetHeight() const bool Texture::IsNative() const { - return mNativeImage != nullptr; + return static_cast(mNativeImage); } bool Texture::ApplyNativeFragmentShader(std::string& shader) { bool modified = false; - if(mNativeImage != nullptr && !shader.empty()) + if(mNativeImage && !shader.empty()) { modified = mNativeImage->ApplyNativeFragmentShader(shader); } diff --git a/dali/internal/render/renderers/render-texture.cpp b/dali/internal/render/renderers/render-texture.cpp index e0f2c1b..0fb1cec 100644 --- a/dali/internal/render/renderers/render-texture.cpp +++ b/dali/internal/render/renderers/render-texture.cpp @@ -273,7 +273,7 @@ void Texture::CreateWithData(Graphics::TextureUsageFlags usage, uint8_t* data, u void Texture::Upload(PixelDataPtr pixelData, const Internal::Texture::UploadParams& params) { - DALI_ASSERT_ALWAYS(mNativeImage == nullptr); + DALI_ASSERT_ALWAYS(!mNativeImage); if(!mGraphicsTexture) { diff --git a/dali/internal/render/renderers/render-texture.h b/dali/internal/render/renderers/render-texture.h index d9223b8..ea8cd13 100644 --- a/dali/internal/render/renderers/render-texture.h +++ b/dali/internal/render/renderers/render-texture.h @@ -128,7 +128,7 @@ public: */ [[nodiscard]] bool IsNativeImage() const { - return mNativeImage; + return static_cast(mNativeImage); } private: diff --git a/dali/internal/update/render-tasks/scene-graph-render-task-list.cpp b/dali/internal/update/render-tasks/scene-graph-render-task-list.cpp index 9aa924f..8139c2b 100644 --- a/dali/internal/update/render-tasks/scene-graph-render-task-list.cpp +++ b/dali/internal/update/render-tasks/scene-graph-render-task-list.cpp @@ -59,7 +59,7 @@ void RenderTaskList::SetRenderMessageDispatcher(RenderMessageDispatcher* renderM void RenderTaskList::AddTask(OwnerPointer& newTask) { - DALI_ASSERT_DEBUG(newTask != NULL && "SceneGraph RenderTask is null"); + DALI_ASSERT_DEBUG(newTask && "SceneGraph RenderTask is null"); DALI_ASSERT_DEBUG(mRenderMessageDispatcher != NULL && "RenderMessageDispatcher is null"); newTask->Initialize(*mRenderMessageDispatcher); diff --git a/dali/public-api/common/intrusive-ptr.h b/dali/public-api/common/intrusive-ptr.h index b566257..9397d14 100644 --- a/dali/public-api/common/intrusive-ptr.h +++ b/dali/public-api/common/intrusive-ptr.h @@ -251,24 +251,17 @@ public: IntrusivePtr(rhs).Swap(*this); } - // IntrusivePtr comparisons - This is a variation of the safe bool idiom + // IntrusivePtr comparisons /** - * @brief Pointer-to-member type. - * - * Objects can be implicitly converted to this for validity checks. - */ - using BooleanType = void (IntrusivePtr::*)() const; - - /** - * @brief Converts an object handle to a BooleanType. + * @brief Converts an object handle to a bool. * * This is useful for checking whether the handle is NULL. * @SINCE_1_0.0 */ - operator BooleanType() const + explicit operator bool() const { - return mPtr ? &IntrusivePtr::ThisIsSaferThanReturningVoidStar : nullptr; + return mPtr != nullptr; } /** @@ -287,14 +280,6 @@ public: private: /** - * @brief Used by the safe bool idiom. - * @SINCE_1_0.0 - */ - void ThisIsSaferThanReturningVoidStar() const - { - } - - /** * @brief Internal swap function. * @SINCE_1_0.0 */ diff --git a/dali/public-api/object/base-handle.cpp b/dali/public-api/object/base-handle.cpp index 121cc0d..e0e612e 100644 --- a/dali/public-api/object/base-handle.cpp +++ b/dali/public-api/object/base-handle.cpp @@ -75,9 +75,9 @@ void BaseHandle::Reset() mObjectHandle = nullptr; } -BaseHandle::operator BaseHandle::BooleanType() const +BaseHandle::operator bool() const { - return mObjectHandle ? &BaseHandle::ThisIsSaferThanReturningVoidStar : nullptr; + return static_cast(mObjectHandle); } bool BaseHandle::operator==(const BaseHandle& rhs) const diff --git a/dali/public-api/object/base-handle.h b/dali/public-api/object/base-handle.h index 35a7873..e275473 100644 --- a/dali/public-api/object/base-handle.h +++ b/dali/public-api/object/base-handle.h @@ -211,21 +211,15 @@ public: */ void Reset(); - // BaseHandle comparisons - This is a variation of the safe bool idiom + // BaseHandle comparisons /** - * @brief Pointer-to-member type. - * Objects can be implicitly converted to this for validity checks. - */ - using BooleanType = void (BaseHandle::*)() const; - - /** - * @brief Converts an handle to a BooleanType. + * @brief Converts an handle to a bool. * * This is useful for checking whether the handle is empty. * @SINCE_1_0.0 */ - operator BooleanType() const; + explicit operator bool() const; /** * @brief Equality operator overload. @@ -265,18 +259,6 @@ private: */ bool DoConnectSignal(ConnectionTrackerInterface* connectionTracker, const std::string& signalName, FunctorDelegate* functorDelegate); -protected: - /** - * @brief Used by the safe bool idiom. - * - * The safe bool idiom basically provides a Boolean test for classes. It validates objects - * in a boolean context without the usual harmful side effects. - * @SINCE_1_0.0 - */ - void ThisIsSaferThanReturningVoidStar() const - { - } - private: IntrusivePtr mObjectHandle; ///< Object this handle points at. }; @@ -295,8 +277,6 @@ inline T DownCast(BaseHandle handle) return T::DownCast(handle); } -// See also BaseHandle::BooleanType() conversion - /** * @brief Equality operator. * @SINCE_1_0.0 @@ -307,7 +287,6 @@ inline T DownCast(BaseHandle handle) template inline bool operator==(const BaseHandle& lhs, const T& rhs) { - // We depart from the safe bool idiom to allow Dali::BaseHandle derived classes to be compared return lhs == static_cast(rhs); } @@ -321,7 +300,6 @@ inline bool operator==(const BaseHandle& lhs, const T& rhs) template inline bool operator!=(const BaseHandle& lhs, const T& rhs) { - // We depart from the safe bool idiom to allow Dali::BaseHandle derived classes to be compared return lhs != static_cast(rhs); } -- 2.7.4