From 8a1107807563841017aca0c8913207086f339fd9 Mon Sep 17 00:00:00 2001 From: Vittorio Romeo Date: Tue, 15 Feb 2022 16:05:26 +0000 Subject: [PATCH] Add move semantics to 'AlResource' --- include/SFML/Audio/AlResource.hpp | 51 +++++++ src/SFML/Audio/AlResource.cpp | 126 ++++++++++++++++- test/Audio/AlResource.cpp | 217 ++++++++++++++++++++++++++++++ test/Audio/Dummy.cpp | 0 test/CMakeLists.txt | 2 +- 5 files changed, 389 insertions(+), 7 deletions(-) create mode 100644 test/Audio/AlResource.cpp delete mode 100644 test/Audio/Dummy.cpp diff --git a/include/SFML/Audio/AlResource.hpp b/include/SFML/Audio/AlResource.hpp index 6cf5eac6c..bf3adbb8b 100644 --- a/include/SFML/Audio/AlResource.hpp +++ b/include/SFML/Audio/AlResource.hpp @@ -31,6 +31,15 @@ #include +namespace sf::priv +{ +//////////////////////////////////////////////////////////// +/// \brief Return OpenAL resource count +/// +//////////////////////////////////////////////////////////// +SFML_AUDIO_API unsigned int getOpenAlResourceCount(); +} + namespace sf { //////////////////////////////////////////////////////////// @@ -52,6 +61,48 @@ protected: /// //////////////////////////////////////////////////////////// ~AlResource(); + + //////////////////////////////////////////////////////////// + /// \brief Copy constructor + /// + //////////////////////////////////////////////////////////// + AlResource(const AlResource& other); + + //////////////////////////////////////////////////////////// + /// \brief Move constructor + /// + //////////////////////////////////////////////////////////// + AlResource(AlResource&& other) noexcept; + + //////////////////////////////////////////////////////////// + /// \brief Copy assignment operator + /// + //////////////////////////////////////////////////////////// + AlResource& operator=(const AlResource& other); + + //////////////////////////////////////////////////////////// + /// \brief Move assignment operator + /// + //////////////////////////////////////////////////////////// + AlResource& operator=(AlResource&& other) noexcept; + +private: + //////////////////////////////////////////////////////////// + /// \brief Increment resource counter + /// + //////////////////////////////////////////////////////////// + void incrementResourceCounter(); + + //////////////////////////////////////////////////////////// + /// \brief Decrement resource counter + /// + //////////////////////////////////////////////////////////// + void decrementResourceCounter(); + + //////////////////////////////////////////////////////////// + // Member data + //////////////////////////////////////////////////////////// + bool m_active; //!< Whether the resource is active or not }; } // namespace sf diff --git a/src/SFML/Audio/AlResource.cpp b/src/SFML/Audio/AlResource.cpp index 773fd72f2..c803c7769 100644 --- a/src/SFML/Audio/AlResource.cpp +++ b/src/SFML/Audio/AlResource.cpp @@ -29,11 +29,13 @@ #include #include #include +#include +#include namespace { - // OpenAL resources counter and its mutex + // OpenAL resource counter and its mutex unsigned int count = 0; std::recursive_mutex mutex; @@ -44,10 +46,122 @@ namespace } +namespace sf::priv +{ +//////////////////////////////////////////////////////////// +/// \brief Return OpenAL resource count +/// +//////////////////////////////////////////////////////////// +unsigned int getOpenAlResourceCount() +{ + std::scoped_lock lock(mutex); + return count; +} + +} + + namespace sf { //////////////////////////////////////////////////////////// -AlResource::AlResource() +AlResource::AlResource() : +m_active(true) +{ + incrementResourceCounter(); +} + + +//////////////////////////////////////////////////////////// +AlResource::~AlResource() +{ + if (m_active) + decrementResourceCounter(); +} + + +//////////////////////////////////////////////////////////// +AlResource::AlResource(const AlResource& other) : +m_active(other.m_active) +{ + if (m_active) + incrementResourceCounter(); +} + + +//////////////////////////////////////////////////////////// +AlResource::AlResource(AlResource&& other) noexcept : +m_active(std::exchange(other.m_active, false)) +{ +} + + +//////////////////////////////////////////////////////////// +AlResource& AlResource::operator=(const AlResource& other) +{ + if (&other == this) + return *this; + + if (m_active && other.m_active) + { + // Both resources are already accounted for, nothing to do + } + else if (!m_active && other.m_active) + { + // '*this' was previously moved-from + m_active = true; + incrementResourceCounter(); + } + else if (m_active && !other.m_active) + { + // 'other' was previously moved-from + m_active = false; + decrementResourceCounter(); + } + else + { + // Both resources are inactive, nothing to do + assert(!m_active && !other.m_active); + } + + return *this; +} + + +//////////////////////////////////////////////////////////// +AlResource& AlResource::operator=(AlResource&& other) noexcept +{ + if (&other == this) + return *this; + + if (m_active && other.m_active) + { + other.m_active = false; + decrementResourceCounter(); + } + else if (!m_active && other.m_active) + { + // '*this' was previously moved-from + m_active = true; + other.m_active = false; + } + else if (m_active && !other.m_active) + { + // 'other' was previously moved-from + m_active = false; + decrementResourceCounter(); + } + else + { + // Both resources are inactive, nothing to do + assert(!m_active && !other.m_active); + } + + return *this; +} + + +//////////////////////////////////////////////////////////// +void AlResource::incrementResourceCounter() { // Protect from concurrent access std::scoped_lock lock(mutex); @@ -57,20 +171,20 @@ AlResource::AlResource() globalDevice = std::make_unique(); // Increment the resources counter - count++; + ++count; } //////////////////////////////////////////////////////////// -AlResource::~AlResource() +void AlResource::decrementResourceCounter() { // Protect from concurrent access std::scoped_lock lock(mutex); // Decrement the resources counter - count--; + --count; - // If there's no more resource alive, we can destroy the device + // If there are no more resources alive, we can destroy the device if (count == 0) globalDevice.reset(); } diff --git a/test/Audio/AlResource.cpp b/test/Audio/AlResource.cpp new file mode 100644 index 000000000..34dd4a7d3 --- /dev/null +++ b/test/Audio/AlResource.cpp @@ -0,0 +1,217 @@ +#include +#include + +#include + +namespace { + +struct TestAlResource : sf::AlResource { }; + +} + +TEST_CASE("sf::AlResource class - [audio]") +{ + SUBCASE("Never used") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Basic ctor/dtor") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Nested ctor/dtor") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + { + TestAlResource r1; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Copy ctor") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + { + TestAlResource r1 = r0; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Move ctor") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + { + TestAlResource r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Copy assignment from active to active") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + TestAlResource r1; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + + r1 = r0; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Copy assignment from inactive to active") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + TestAlResource r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + r1 = r0; + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Move assignment from active to active") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + TestAlResource r1; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + + r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Move assignment from inactive to active") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + TestAlResource r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Copy assignment from active to inactive") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + TestAlResource r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + r1 = r0; + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Move assignment from active to inactive") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + TestAlResource r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 1u); + + r1 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } + + SUBCASE("Copy assignment from inactive to inactive") + { + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + + { + TestAlResource r0; + TestAlResource r1; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + + TestAlResource r2 = std::move(r0); + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + + TestAlResource r3 = std::move(r1); + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + + r0 = r1; + CHECK(sf::priv::getOpenAlResourceCount() == 2u); + } + + CHECK(sf::priv::getOpenAlResourceCount() == 0u); + } +} diff --git a/test/Audio/Dummy.cpp b/test/Audio/Dummy.cpp deleted file mode 100644 index e69de29bb..000000000 diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index d366dadff..04e6b4291 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -53,7 +53,7 @@ endif() if(SFML_BUILD_AUDIO) SET(AUDIO_SRC - "${SRCROOT}/Audio/Dummy.cpp" # TODO: Remove when there are real tests + "${SRCROOT}/Audio/AlResource.cpp" ) sfml_add_test(test-sfml-audio "${AUDIO_SRC}" SFML::Audio) target_link_libraries(test-sfml-audio PRIVATE sfml-test-main)