From 0f7e1e8fadfb6ffa74dc5f74c5d2fa35ff16721e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 11 Aug 2019 13:38:10 +0200 Subject: [PATCH] Deprecate ResourceManager::instance(). If I would have done this a year ago, I could have it removed by now. Well. Gotta look forward to 2020, then. --- doc/changelog.dox | 3 +++ doc/snippets/Magnum.cpp | 2 +- src/Magnum/DebugTools/ResourceManager.cpp | 23 ++++++++++++++++++- src/Magnum/DebugTools/ResourceManager.h | 7 ++++++ src/Magnum/ResourceManager.h | 10 +++++++- src/Magnum/ResourceManager.hpp | 2 ++ src/Magnum/Test/CMakeLists.txt | 23 +++++++++++-------- .../Test/ResourceManagerLocalInstanceTest.cpp | 3 ++- .../ResourceManagerLocalInstanceTestLib.cpp | 2 ++ src/Magnum/Test/ResourceManagerTest.cpp | 4 ++-- 10 files changed, 64 insertions(+), 15 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index b84f7d88e..0ba2a5d22 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -585,6 +585,9 @@ See also: - Passing @cpp nullptr @ce to @ref ImageView constructors is deprecated and will print a warning at runtime. Use a constructor without the @p data parameter instead. +- The @cpp ResourceManager::instance() @ce singleton is deprecated as it + makes some use cases harder than they should be. Make your own singleton + or explicitly pass a @ref ResourceManager reference around instead. - @ref Platform::BasicScreen::application() now returns a reference instead of a pointer and together with @ref Platform::BasicScreen::redraw() checks that the screen is actually added to the application instead of returning diff --git a/doc/snippets/Magnum.cpp b/doc/snippets/Magnum.cpp index 6594d4c9c..b5dd5e7df 100644 --- a/doc/snippets/Magnum.cpp +++ b/doc/snippets/Magnum.cpp @@ -233,7 +233,7 @@ struct MyShader: GL::AbstractShaderProgram { void bindTexture(GL::Texture2D&) {} }; /* [ResourceManager-fill] */ -MyResourceManager& manager = MyResourceManager::instance(); +MyResourceManager manager; Resource texture{manager.get("texture")}; Resource shader = manager.get("shader"); diff --git a/src/Magnum/DebugTools/ResourceManager.cpp b/src/Magnum/DebugTools/ResourceManager.cpp index df5d85afb..aaad66ee1 100644 --- a/src/Magnum/DebugTools/ResourceManager.cpp +++ b/src/Magnum/DebugTools/ResourceManager.cpp @@ -41,11 +41,32 @@ namespace Implementation { namespace DebugTools { +namespace { + #ifdef CORRADE_BUILD_MULTITHREADED + CORRADE_THREAD_LOCAL + #endif + ResourceManager* resourceManagerInstance = nullptr; +} + +ResourceManager& ResourceManager::instance() { + CORRADE_ASSERT(resourceManagerInstance, + "DebugTools::ResourceManager::instance(): no instance exists", + *resourceManagerInstance); + return *resourceManagerInstance; +} + ResourceManager::ResourceManager() { + CORRADE_ASSERT(!resourceManagerInstance, + "DebugTools::ResourceManager: another instance is already created", ); + resourceManagerInstance = this; + setFallback(new ForceRendererOptions); setFallback(new ObjectRendererOptions); } -ResourceManager::~ResourceManager() = default; +ResourceManager::~ResourceManager() { + CORRADE_INTERNAL_ASSERT(resourceManagerInstance == this); + resourceManagerInstance = nullptr; +} }} diff --git a/src/Magnum/DebugTools/ResourceManager.h b/src/Magnum/DebugTools/ResourceManager.h index 422c83fed..6a1914758 100644 --- a/src/Magnum/DebugTools/ResourceManager.h +++ b/src/Magnum/DebugTools/ResourceManager.h @@ -65,6 +65,13 @@ information. class MAGNUM_DEBUGTOOLS_EXPORT ResourceManager: public Magnum::ResourceManager { public: + /** + * @brief Global instance + * + * Assumes that the instance exists. + */ + static ResourceManager& instance(); + explicit ResourceManager(); ~ResourceManager(); }; diff --git a/src/Magnum/ResourceManager.h b/src/Magnum/ResourceManager.h index 88cfe0b1d..d2d6f21ab 100644 --- a/src/Magnum/ResourceManager.h +++ b/src/Magnum/ResourceManager.h @@ -246,12 +246,14 @@ Basic usage is: template implementation file. */ template class ResourceManager: private Implementation::ResourceManagerImplementation... { public: + #ifdef MAGNUM_BUILD_DEPRECATED /** * @brief Global instance * * Assumes that the instance exists. */ - static ResourceManager& instance(); + static CORRADE_DEPRECATED("implicit ResourceManager singleton is deprecated, make your own or pass a reference around instead") ResourceManager& instance(); + #endif /** * @brief Constructor @@ -625,23 +627,29 @@ template inline ResourceManagerData::Data::~Data() { } +#ifdef MAGNUM_BUILD_DEPRECATED template ResourceManager& ResourceManager::instance() { CORRADE_ASSERT(Implementation::ResourceManagerImplementation::internalInstance(), "ResourceManager::instance(): no instance exists", static_cast&>(*Implementation::ResourceManagerImplementation::internalInstance())); return static_cast&>(*Implementation::ResourceManagerImplementation::internalInstance()); } +#endif template ResourceManager::ResourceManager() { + #ifdef MAGNUM_BUILD_DEPRECATED CORRADE_ASSERT(!Implementation::ResourceManagerImplementation::internalInstance(), "ResourceManager::ResourceManager(): another instance is already created", ); Implementation::ResourceManagerImplementation::internalInstance() = this; + #endif } template ResourceManager::~ResourceManager() { freeLoaders(typename Implementation::ResourceManagerImplementation::TypePack{}); + #ifdef MAGNUM_BUILD_DEPRECATED CORRADE_INTERNAL_ASSERT(Implementation::ResourceManagerImplementation::internalInstance() == this); Implementation::ResourceManagerImplementation::internalInstance() = nullptr; + #endif } } diff --git a/src/Magnum/ResourceManager.hpp b/src/Magnum/ResourceManager.hpp index a3bd11a4b..a283b8373 100644 --- a/src/Magnum/ResourceManager.hpp +++ b/src/Magnum/ResourceManager.hpp @@ -40,6 +40,7 @@ namespace Magnum { namespace Implementation { +#ifdef MAGNUM_BUILD_DEPRECATED template #ifndef _MSC_VER CORRADE_VISIBILITY_EXPORT @@ -48,6 +49,7 @@ ResourceManager*& ResourceManagerLocalInstanceImplementation static ResourceManager* _instance(nullptr); return _instance; } +#endif }} diff --git a/src/Magnum/Test/CMakeLists.txt b/src/Magnum/Test/CMakeLists.txt index 2e94fa982..1cf8df71b 100644 --- a/src/Magnum/Test/CMakeLists.txt +++ b/src/Magnum/Test/CMakeLists.txt @@ -35,13 +35,6 @@ corrade_add_test(ResourceManagerTest ResourceManagerTest.cpp LIBRARIES Magnum) target_compile_definitions(ResourceManagerTest PRIVATE "CORRADE_GRACEFUL_ASSERT") corrade_add_test(SamplerTest SamplerTest.cpp LIBRARIES MagnumTestLib) -add_library(ResourceManagerLocalInstanceTestLib ${SHARED_OR_STATIC} ResourceManagerLocalInstanceTestLib.cpp) -target_link_libraries(ResourceManagerLocalInstanceTestLib Magnum) -if(NOT BUILD_STATIC) - target_compile_definitions(ResourceManagerLocalInstanceTestLib PRIVATE "ResourceManagerLocalInstanceTestLib_EXPORTS") -endif() -corrade_add_test(ResourceManagerLocalInstanceTest ResourceManagerLocalInstanceTest.cpp LIBRARIES Magnum ResourceManagerLocalInstanceTestLib) - corrade_add_test(TagsTest TagsTest.cpp LIBRARIES Magnum) set_target_properties( @@ -52,8 +45,20 @@ set_target_properties( PixelFormatTest PixelStorageTest ResourceManagerTest - ResourceManagerLocalInstanceTestLib - ResourceManagerLocalInstanceTest SamplerTest TagsTest PROPERTIES FOLDER "Magnum/Test") + +if(MAGNUM_BUILD_DEPRECATED) + add_library(ResourceManagerLocalInstanceTestLib ${SHARED_OR_STATIC} ResourceManagerLocalInstanceTestLib.cpp) + target_link_libraries(ResourceManagerLocalInstanceTestLib Magnum) + if(NOT BUILD_STATIC) + target_compile_definitions(ResourceManagerLocalInstanceTestLib PRIVATE "ResourceManagerLocalInstanceTestLib_EXPORTS") + endif() + corrade_add_test(ResourceManagerLocalInstanceTest ResourceManagerLocalInstanceTest.cpp LIBRARIES Magnum ResourceManagerLocalInstanceTestLib) + + set_target_properties( + ResourceManagerLocalInstanceTestLib + ResourceManagerLocalInstanceTest + PROPERTIES FOLDER "Magnum/Test") +endif() diff --git a/src/Magnum/Test/ResourceManagerLocalInstanceTest.cpp b/src/Magnum/Test/ResourceManagerLocalInstanceTest.cpp index c11c3d3aa..244900981 100644 --- a/src/Magnum/Test/ResourceManagerLocalInstanceTest.cpp +++ b/src/Magnum/Test/ResourceManagerLocalInstanceTest.cpp @@ -42,9 +42,10 @@ ResourceManagerLocalInstanceTest::ResourceManagerLocalInstanceTest() { } void ResourceManagerLocalInstanceTest::instance() { + CORRADE_IGNORE_DEPRECATED_PUSH ResourceManagerWithLocalInstance::instance().set("another", 13); - CORRADE_COMPARE(&manager.staticInstance, &manager.instance()); + CORRADE_IGNORE_DEPRECATED_POP CORRADE_COMPARE(manager.count(), 2); CORRADE_COMPARE(manager.state("integer"), ResourceState::Final); } diff --git a/src/Magnum/Test/ResourceManagerLocalInstanceTestLib.cpp b/src/Magnum/Test/ResourceManagerLocalInstanceTestLib.cpp index 033fbb091..16621a8cb 100644 --- a/src/Magnum/Test/ResourceManagerLocalInstanceTestLib.cpp +++ b/src/Magnum/Test/ResourceManagerLocalInstanceTestLib.cpp @@ -35,9 +35,11 @@ namespace Implementation { namespace Test { +CORRADE_IGNORE_DEPRECATED_PUSH ResourceManagerWithLocalInstance::ResourceManagerWithLocalInstance(): staticInstance(static_cast(instance())) { /* Add some stuff */ set("integer", 42); } +CORRADE_IGNORE_DEPRECATED_POP }} diff --git a/src/Magnum/Test/ResourceManagerTest.cpp b/src/Magnum/Test/ResourceManagerTest.cpp index acc23710c..1ee6424a6 100644 --- a/src/Magnum/Test/ResourceManagerTest.cpp +++ b/src/Magnum/Test/ResourceManagerTest.cpp @@ -316,7 +316,7 @@ void ResourceManagerTest::clearWhileReferenced() { void ResourceManagerTest::loader() { class IntResourceLoader: public AbstractResourceLoader { public: - IntResourceLoader(): resource(ResourceManager::instance().get("data")) {} + IntResourceLoader(ResourceManager& instance): resource{instance.get("data")} {} void load() { set("hello", Containers::pointer(773), ResourceDataState::Final, ResourcePolicy::Resident); @@ -338,7 +338,7 @@ void ResourceManagerTest::loader() { { ResourceManager rm; - Containers::Pointer loaderPtr{Containers::InPlaceInit}; + Containers::Pointer loaderPtr{Containers::InPlaceInit, rm}; IntResourceLoader& loader = *loaderPtr; rm.setLoader(std::move(loaderPtr));