From 7e8d90a436fe57ee236ea854ef69cab828b25118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 5 Apr 2015 23:37:44 +0200 Subject: [PATCH] Better fix to ensure a single static ResourceManager instance. I'm not exactly sure where was the actual problem, but it seems to be related to definition of templated function with static member variables in header file in combination with dynamic libraries, causing multiple definitions of a symbol which did or did not lead to compile error and/or things breaking in horribly unpredictable way. Previously it was done via (undocumented) macros, depending on include order, which is great until it breaks horribly after random unrelated changes. Currently it is done via black template magic and I'm still afraid of including it in the public documentation. There should be no visible change to the end user except for possibly one less randomly weird bug (#79? couldn't reproduce). ... well, looking at the diff, the solution is still an ugly **********, but at least it seems to work. --- src/Magnum/CMakeLists.txt | 1 + src/Magnum/DebugTools/ResourceManager.cpp | 11 ++-- src/Magnum/DebugTools/ResourceManager.h | 13 +--- src/Magnum/ResourceManager.h | 74 +++++++++++++++-------- src/Magnum/ResourceManager.hpp | 46 ++++++++++++++ 5 files changed, 100 insertions(+), 45 deletions(-) create mode 100644 src/Magnum/ResourceManager.hpp diff --git a/src/Magnum/CMakeLists.txt b/src/Magnum/CMakeLists.txt index 9d7e0bde0..f3fa54aaf 100644 --- a/src/Magnum/CMakeLists.txt +++ b/src/Magnum/CMakeLists.txt @@ -112,6 +112,7 @@ set(Magnum_HEADERS Renderer.h Resource.h ResourceManager.h + ResourceManager.hpp SampleQuery.h Sampler.h Shader.h diff --git a/src/Magnum/DebugTools/ResourceManager.cpp b/src/Magnum/DebugTools/ResourceManager.cpp index 7e8d2f632..cec160fd4 100644 --- a/src/Magnum/DebugTools/ResourceManager.cpp +++ b/src/Magnum/DebugTools/ResourceManager.cpp @@ -23,25 +23,22 @@ DEALINGS IN THE SOFTWARE. */ -#define MAGNUM_RESOURCEMANAGER_DEFINE_INTERNALINSTANCE - #include "ResourceManager.h" #include "Magnum/AbstractShaderProgram.h" #include "Magnum/Buffer.h" #include "Magnum/Mesh.h" #include "Magnum/MeshView.h" +#include "Magnum/ResourceManager.hpp" #include "Magnum/DebugTools/ForceRenderer.h" #include "Magnum/DebugTools/ObjectRenderer.h" #include "Magnum/DebugTools/ShapeRenderer.h" namespace Magnum { -template class -#if defined(CORRADE_TARGET_WINDOWS) && _MSC_VER -MAGNUM_DEBUGTOOLS_EXPORT -#endif -ResourceManager; +namespace Implementation { + template MAGNUM_DEBUGTOOLS_EXPORT ResourceManager*& ResourceManagerLocalInstanceImplementation::internalInstance(); +} namespace DebugTools { diff --git a/src/Magnum/DebugTools/ResourceManager.h b/src/Magnum/DebugTools/ResourceManager.h index 561399a1a..e2fe70a24 100644 --- a/src/Magnum/DebugTools/ResourceManager.h +++ b/src/Magnum/DebugTools/ResourceManager.h @@ -29,9 +29,6 @@ * @brief Class @ref Magnum::DebugTools::ResourceManager */ -#ifndef MAGNUM_RESOURCEMANAGER_DEFINE_INTERNALINSTANCE -#define MAGNUM_RESOURCEMANAGER_DONT_DEFINE_INTERNALINSTANCE -#endif #include "Magnum/ResourceManager.h" #include "Magnum/Magnum.h" @@ -42,14 +39,6 @@ namespace Magnum { -/** @todo Do the listing in one place, not five thousand! */ - -#ifndef CORRADE_TARGET_WINDOWS -extern template ResourceManager MAGNUM_DEBUGTOOLS_EXPORT *& ResourceManager::internalInstance(); -#else -extern template class MAGNUM_DEBUGTOOLS_EXPORT ResourceManager; -#endif - namespace DebugTools { /** @@ -58,7 +47,7 @@ namespace DebugTools { Stores various data used by debug renderers. See @ref debug-tools for more information. */ -class MAGNUM_DEBUGTOOLS_EXPORT ResourceManager: public Magnum::ResourceManager { +class MAGNUM_DEBUGTOOLS_EXPORT ResourceManager: public Magnum::ResourceManager { public: explicit ResourceManager(); ~ResourceManager(); diff --git a/src/Magnum/ResourceManager.h b/src/Magnum/ResourceManager.h index 9fc0516b5..06dc4642f 100644 --- a/src/Magnum/ResourceManager.h +++ b/src/Magnum/ResourceManager.h @@ -155,6 +155,29 @@ template class ResourceManagerData { std::size_t _lastChange; }; +/* Helper class for defining which real types are in the type pack */ +template struct ResourceTypePack {}; + +/* Common resource manager implementation with inline internal instance (for + use in user code), definition of internalInstance() is in this header */ +template struct ResourceManagerInlineInstanceImplementation { + static ResourceManager*& internalInstance(); +}; +template struct ResourceManagerImplementation: ResourceManagerInlineInstanceImplementation, ResourceManagerData... { + typedef ResourceTypePack TypePack; +}; + +/* Resource manager implementation with file-local internal instance (for use + in code where the manager is used across library boundaries), definition + of internalInstance() is in ResourceManager.hpp, see it for usage details */ +template struct ResourceManagerLocalInstanceImplementation { + static ResourceManager*& internalInstance(); +}; +struct ResourceManagerLocalInstance; +template struct ResourceManagerImplementation: ResourceManagerLocalInstanceImplementation, ResourceManagerData... { + typedef ResourceTypePack TypePack; +}; + } /** @@ -224,7 +247,7 @@ cube->draw(*shader); /* Due to too much work involved with explicit template instantiation (all Resource combinations, all ResourceManagerData...), this class doesn't have template implementation file. */ -template class ResourceManager: private Implementation::ResourceManagerData... { +template class ResourceManager: private Implementation::ResourceManagerImplementation... { public: /** * @brief Global instance @@ -366,7 +389,7 @@ template class ResourceManager: private Implementation::Resource * @return Reference to self (for method chaining) */ ResourceManager& free() { - freeInternal(); + freeInternal(typename Implementation::ResourceManagerImplementation::TypePack{}); return *this; } @@ -390,7 +413,7 @@ template class ResourceManager: private Implementation::Resource * referenced. */ ResourceManager& clear() { - clearInternal(); + clearInternal(typename Implementation::ResourceManagerImplementation::TypePack{}); return *this; } @@ -419,35 +442,31 @@ template class ResourceManager: private Implementation::Resource } private: - template void freeInternal() { + template void freeInternal(Implementation::ResourceTypePack) { free(); - freeInternal(); + freeInternal(Implementation::ResourceTypePack{}); } - template void freeInternal() const {} + void freeInternal(Implementation::ResourceTypePack<>) const {} - template void clearInternal() { + template void clearInternal(Implementation::ResourceTypePack) { clear(); - clearInternal(); + clearInternal(Implementation::ResourceTypePack{}); } - template void clearInternal() const {} + void clearInternal(Implementation::ResourceTypePack<>) const {} - template void freeLoaders() { + template void freeLoaders(Implementation::ResourceTypePack) { Implementation::ResourceManagerData::freeLoader(); - freeLoaders(); + freeLoaders(Implementation::ResourceTypePack{}); } - template void freeLoaders() const {} - - static ResourceManager*& internalInstance(); + void freeLoaders(Implementation::ResourceTypePack<>) const {} }; -#ifndef MAGNUM_RESOURCEMANAGER_DONT_DEFINE_INTERNALINSTANCE -template ResourceManager*& ResourceManager::internalInstance() { +namespace Implementation { + +template ResourceManager*& ResourceManagerInlineInstanceImplementation::internalInstance() { static ResourceManager* _instance(nullptr); return _instance; } -#endif - -namespace Implementation { template void safeDelete(T* data) { static_assert(sizeof(T) > 0, "Cannot delete pointer to incomplete type"); @@ -588,19 +607,22 @@ template inline ResourceManagerData::Data::~Data() { } template ResourceManager& ResourceManager::instance() { - CORRADE_ASSERT(internalInstance(), "ResourceManager::instance(): no instance exists", *internalInstance()); - return *internalInstance(); + CORRADE_ASSERT(Implementation::ResourceManagerImplementation::internalInstance(), + "ResourceManager::instance(): no instance exists", + static_cast&>(*Implementation::ResourceManagerImplementation::internalInstance())); + return static_cast&>(*Implementation::ResourceManagerImplementation::internalInstance()); } template ResourceManager::ResourceManager() { - CORRADE_ASSERT(!internalInstance(), "ResourceManager::ResourceManager(): another instance is already created", ); - internalInstance() = this; + CORRADE_ASSERT(!Implementation::ResourceManagerImplementation::internalInstance(), + "ResourceManager::ResourceManager(): another instance is already created", ); + Implementation::ResourceManagerImplementation::internalInstance() = this; } template ResourceManager::~ResourceManager() { - freeLoaders(); - CORRADE_INTERNAL_ASSERT(internalInstance() == this); - internalInstance() = nullptr; + freeLoaders(typename Implementation::ResourceManagerImplementation::TypePack{}); + CORRADE_INTERNAL_ASSERT(Implementation::ResourceManagerImplementation::internalInstance() == this); + Implementation::ResourceManagerImplementation::internalInstance() = nullptr; } } diff --git a/src/Magnum/ResourceManager.hpp b/src/Magnum/ResourceManager.hpp new file mode 100644 index 000000000..44ffc3d02 --- /dev/null +++ b/src/Magnum/ResourceManager.hpp @@ -0,0 +1,46 @@ +/* + This file is part of Magnum. + + Copyright © 2010, 2011, 2012, 2013, 2014, 2015 + Vladimír Vondruš + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included + in all copies or substantial portions of the Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. +*/ + +#include "ResourceManager.h" + +/* + File-local definition of ResourceManager instance holder for use in cases + where the class is used across library boundaries, in which case additional + care must be done to ensure a single static instance. + + Usage: typedef the resource manager with Implementation::ResourceManagerLocalInstance + as a first type and then include this file in a _single_ *.cpp file. + + This symbol is always exported. +*/ + +namespace Magnum { namespace Implementation { + +template CORRADE_VISIBILITY_EXPORT ResourceManager*& ResourceManagerLocalInstanceImplementation::internalInstance() { + static ResourceManager* _instance(nullptr); + return _instance; +} + +}}