From c3a6379575f89ebcc8952d0c3053d8b2094b03a3 Mon Sep 17 00:00:00 2001 From: Jim Carroll Date: Tue, 22 Feb 2011 21:13:48 -0500 Subject: [PATCH] Cleaned up some of the grammar and spelling in the comments, thanks in large part to a patch submitted by 'PaulePanter.' Also changed the globals fix to use boost::shared_ptr rather than the custom Reference counting. Thanks to elupus insisting that the globals fix be done with macros, this turned out to be fairly simple. --- project/VS2010Express/XBMC.vcxproj | 6 +- project/VS2010Express/XBMC.vcxproj.filters | 10 +- xbmc/SectionLoader.h | 2 +- xbmc/guilib/GraphicContext.h | 2 +- xbmc/settings/AdvancedSettings.h | 2 +- xbmc/utils/CharsetConverter.h | 2 +- xbmc/utils/GlobalsHandling.h | 97 +++++++++++++------ xbmc/utils/Makefile | 1 - xbmc/utils/ReferenceCounting.cpp | 71 -------------- xbmc/utils/ReferenceCounting.h | 107 --------------------- xbmc/utils/log.h | 2 +- xbmc/windowing/X11/WinSystemX11GL.h | 2 +- xbmc/windowing/egl/WinSystemEGL.h | 2 +- xbmc/windowing/osx/WinSystemOSXGL.h | 2 +- xbmc/windowing/windows/WinSystemWin32DX.h | 2 +- xbmc/windowing/windows/WinSystemWin32GL.h | 2 +- 16 files changed, 82 insertions(+), 230 deletions(-) delete mode 100644 xbmc/utils/ReferenceCounting.cpp delete mode 100644 xbmc/utils/ReferenceCounting.h diff --git a/project/VS2010Express/XBMC.vcxproj b/project/VS2010Express/XBMC.vcxproj index b05e4b04d9..3b73b7550a 100644 --- a/project/VS2010Express/XBMC.vcxproj +++ b/project/VS2010Express/XBMC.vcxproj @@ -1,4 +1,4 @@ - + @@ -686,7 +686,6 @@ - @@ -1517,7 +1516,6 @@ - @@ -1999,4 +1997,4 @@ - \ No newline at end of file + diff --git a/project/VS2010Express/XBMC.vcxproj.filters b/project/VS2010Express/XBMC.vcxproj.filters index 1877c1d6a8..40aef54b4a 100644 --- a/project/VS2010Express/XBMC.vcxproj.filters +++ b/project/VS2010Express/XBMC.vcxproj.filters @@ -1,4 +1,4 @@ - + @@ -2456,9 +2456,6 @@ cores\VideoRenderers - - utils - @@ -4911,9 +4908,6 @@ cores\VideoRenderers - - utils - utils @@ -4933,4 +4927,4 @@ win32 - \ No newline at end of file + diff --git a/xbmc/SectionLoader.h b/xbmc/SectionLoader.h index 805866dcfb..b7f68cbe9b 100644 --- a/xbmc/SectionLoader.h +++ b/xbmc/SectionLoader.h @@ -27,7 +27,7 @@ // forward class LibraryLoader; -class CSectionLoader : public virtual xbmcutil::Referenced +class CSectionLoader { public: class CSection diff --git a/xbmc/guilib/GraphicContext.h b/xbmc/guilib/GraphicContext.h index 8bc1d4205a..1936a364bd 100644 --- a/xbmc/guilib/GraphicContext.h +++ b/xbmc/guilib/GraphicContext.h @@ -61,7 +61,7 @@ enum VIEW_TYPE { VIEW_TYPE_NONE = 0, VIEW_TYPE_MAX }; -class CGraphicContext : public CCriticalSection, public virtual xbmcutil::Referenced +class CGraphicContext : public CCriticalSection { public: CGraphicContext(void); diff --git a/xbmc/settings/AdvancedSettings.h b/xbmc/settings/AdvancedSettings.h index acc92fab1c..571b3ad796 100644 --- a/xbmc/settings/AdvancedSettings.h +++ b/xbmc/settings/AdvancedSettings.h @@ -60,7 +60,7 @@ struct RefreshOverride typedef std::vector SETTINGS_TVSHOWLIST; -class CAdvancedSettings : public virtual xbmcutil::Referenced +class CAdvancedSettings { public: CAdvancedSettings(); diff --git a/xbmc/utils/CharsetConverter.h b/xbmc/utils/CharsetConverter.h index a25cd6ae65..da34b2240e 100644 --- a/xbmc/utils/CharsetConverter.h +++ b/xbmc/utils/CharsetConverter.h @@ -28,7 +28,7 @@ #include -class CCharsetConverter : public virtual xbmcutil::Referenced +class CCharsetConverter { public: CCharsetConverter(); diff --git a/xbmc/utils/GlobalsHandling.h b/xbmc/utils/GlobalsHandling.h index c18ad3c8b3..6961d7be77 100644 --- a/xbmc/utils/GlobalsHandling.h +++ b/xbmc/utils/GlobalsHandling.h @@ -21,13 +21,13 @@ #pragma once -#include "utils/ReferenceCounting.h" +#include "boost/shared_ptr.hpp" /** * This file contains the pattern for moving "globals" from the BSS Segment to the heap. * A note on usage of this pattern for globals replacement: * - * This pattern uses a singleton pattern and some compiler/cpreprocessor sugar to allow + * This pattern uses a singleton pattern and some compiler/C preprocessor sugar to allow * "global" variables to be lazy instantiated and initialized and moved from the BSS segment * to the heap (that is, they are instantiated on the heap when they are first used rather * than relying on the startup code to initialize the BSS segment). This eliminates the @@ -40,8 +40,6 @@ * pointer in the header file of the global class (did you ever think you'd see a file scope * 'static' variable in a header file - on purpose?) * - * For more details see xbmc/utils/ReferenceCounting.h/cpp. - * * There are two different ways to use this pattern when replacing global variables. * The selection of which one to use depends on whether or not there is a possiblity * that the code in the .cpp file for the global can be executed from a static method @@ -49,26 +47,26 @@ * * The (at least) two ways to do this: * - * 1) You can use the reference object Referenced::ref to access the global variable. + * 1) You can use the reference object boost::shared_ptr to access the global variable. * * This would be the preferred means since it is (very slightly) more efficent than * the alternative. To use this pattern you replace standard static references to - * the global with access through the reference. If you use the c-preprocessor to + * the global with access through the reference. If you use the C preprocessor to * do this for you can put the following code in the header file where the global's * class is declared: * - * static xbmcutil::Referenced::ref g_globalVariableRef(xbmcutil::GlobalsSingleton::getInstance); - * #define g_globalVariable (g_globalVariableRef.getRef()) + * static boost::shared_ptr g_globalVariableRef(xbmcutil::GlobalsSingleton::getInstance()); + * #define g_globalVariable (*(g_globalVariableRef.get())) * * Note what this does. In every file that includes this header there will be a *static* - * instance of the Referenced::ref smart pointer. This effectively + * instance of the boost::shared_ptr smart pointer. This effectively * reference counts the singleton from every compilation unit (ie, object code file that * results from a compilation of a .c/.cpp file) that references this global directly. * * There is a problem with this, however. Keep in mind that the instance of the smart pointer * (being in the BSS segment of the compilation unit) is ITSELF an object that depends on * the BSS segment initialization in order to be initialized with an instance from the - * Singleton. That means, depending on the code structure, it is possible to get into a + * singleton. That means, depending on the code structure, it is possible to get into a * circumstance where the above #define could be exercised PRIOR TO the setting of the * value of the smart pointer. * @@ -81,15 +79,15 @@ * Because of the "indirectly" in the above statement, this situation can be difficult to * determine beforehand. * - * 2) Alternately, when you KNOW that the global variable can suffer from the above described - * problem, you can restrict all access to the varaible to the singleton by changing + * 2) Alternatively, when you KNOW that the global variable can suffer from the above described + * problem, you can restrict all access to the variable to the singleton by changing * the #define to: * * #define g_globalVariable (*(xbmcutil::Singleton::getInstance())) * * A few things to note about this. First, this separates the reference counting aspect * from the access aspect of this solution. The smart pointers are no longer used for - * reference, only for access. Secondly, all access is through the Singleton directly + * access, only for reference counting. Secondly, all access is through the singleton directly * so there is no reliance on the state of the BSS segment for the code to operate * correctly. * @@ -102,6 +100,12 @@ namespace xbmcutil { /** + * This class is an implementation detail of the macros defined below and + * is NOT meant to be used as a general purpose utility. IOW, DO NOT USE THIS + * CLASS to support a general singleton design pattern, it's specialized + * for solving the initialization/finalization order/dependency problem + * with global variables and should only be used via the macros below. + * * Currently THIS IS NOT THREAD SAFE! Why not just add a lock you ask? * Because this singleton is used to initialize global variables and * there is an issue with having the lock used prior to its @@ -111,29 +115,64 @@ namespace xbmcutil * * Therefore this hack depends on the fact that compilation unit global/static * initialization is done in a single thread. - * - * In the spirit of making changes incrementally I've opted to not add Atomic* - * locking to this class yet. It doesn't need it (yet) as it's only called - * from the static initialization thread prior to main. */ template class GlobalsSingleton { - static T* instance; + /** + * Is it possible that getInstance can be called prior to the shared_ptr 'instance' + * being initialized as a global? If so, then the shared_ptr constructor would + * effectively 'reset' the shared pointer after it had been set by the prior + * getInstance call, and a second instance would be created. We really don't + * want this to happen so 'instance' is a pointer to a smart pointer so that + * we can deterministally handle its construction. + */ + static boost::shared_ptr* instance; + + /** + * See 'getQuick' below. + */ + static T* quick; public: - static T* getInstance() + + /** + * Retrieve an instance of the singleton using a shared pointer for + * referenece counting. + */ + inline static boost::shared_ptr getInstance() { - if (instance == NULL) - instance = new T; - return instance; + if (!instance) + { + if (!quick) + quick = new T; + instance = new boost::shared_ptr(quick); + } + return *instance; } + + /** + * This is for quick access when using form (2) of the pattern. Before 'mdd' points + * it out, this might be a case of 'solving problems we don't have' but this access + * is used frequently within the event loop so any help here should benefit the + * overall performance and there is nothing complicated or tricky here and not + * a lot of code to maintain. + */ + inline static T* getQuick() + { + if (!quick) + quick = new T; + + return quick; + } + }; - template T* GlobalsSingleton::instance; + template boost::shared_ptr* GlobalsSingleton::instance; + template T* GlobalsSingleton::quick; } /** * For pattern (2) above, you can use the following macro. This pattern is safe to - * use in all cases but may be very slightly less efficent. + * use in all cases but may be very slightly less efficient. * * Also, you must also use a #define to replace the actual global variable since * there's no way to use a macro to add a #define. An example would be: @@ -143,7 +182,7 @@ namespace xbmcutil * */ #define XBMC_GLOBAL_REF(classname,g_variable) \ - static xbmcutil::Referenced::ref g_variable##Ref(xbmcutil::GlobalsSingleton::getInstance) + static boost::shared_ptr g_variable##Ref(xbmcutil::GlobalsSingleton::getInstance()) /** * This declares the actual use of the variable. It needs to be used in another #define @@ -151,14 +190,14 @@ namespace xbmcutil * * #define g_variable XBMC_GLOBAL_USE(classname) */ -#define XBMC_GLOBAL_USE(classname) (*(xbmcutil::GlobalsSingleton::getInstance())) +#define XBMC_GLOBAL_USE(classname) (*(xbmcutil::GlobalsSingleton::getQuick())) /** - * For pattern (1) above, you can use the following macro. WARNING: this should only + * For pattern (1) above, you can use the following macro. WARNING: This should only * be used when the global in question is never accessed, directly or indirectly, from - * a static method called (again, directly or indirectly) durring startup or shutdown. + * a static method called (again, directly or indirectly) during startup or shutdown. */ #define XBMC_GLOBAL(classname,g_variable) \ XBMC_GLOBAL_REF(classname,g_variable); \ - static classname & g_variable = g_variable##Ref.getRef() + static classname & g_variable = (*(g_variable##Ref.get())) diff --git a/xbmc/utils/Makefile b/xbmc/utils/Makefile index b9eb33bac7..016f0c99c3 100644 --- a/xbmc/utils/Makefile +++ b/xbmc/utils/Makefile @@ -31,7 +31,6 @@ SRCS=AlarmClock.cpp \ PCMRemap.cpp \ PerformanceSample.cpp \ PerformanceStats.cpp \ - ReferenceCounting.cpp \ RegExp.cpp \ RingBuffer.cpp \ RssReader.cpp \ diff --git a/xbmc/utils/ReferenceCounting.cpp b/xbmc/utils/ReferenceCounting.cpp deleted file mode 100644 index 2f8c597ac5..0000000000 --- a/xbmc/utils/ReferenceCounting.cpp +++ /dev/null @@ -1,71 +0,0 @@ -/* - * Copyright (C) 2005-2011 Team XBMC - * http://www.xbmc.org - * - * This Program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2, or (at your option) - * any later version. - * - * This Program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with XBMC; see the file COPYING. If not, write to - * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA. - * http://www.gnu.org/copyleft/gpl.html - * - */ - -#include "threads/Atomics.h" -#include "ReferenceCounting.h" - -// Comment in this #define in order to see REFCNT events in the log. -//#define LOG_LIFECYCLE_EVENTS - -#if defined(LOG_LIFECYCLE_EVENTS) || defined(DEBUG_REFS) -#include "utils/log.h" -#endif - -namespace xbmcutil -{ - Referenced::~Referenced() { /* place for the vtab */ } - - void Referenced::Release() const - { - long ct = AtomicDecrement((long*)&refs); -#ifdef LOG_LIFECYCLE_EVENTS - CLog::Log(LOGDEBUG,"REFCNT decrementing to %ld on 0x%lx", ct, (long)(((void*)this))); -#endif - -#ifndef DEBUG_REFS - if(ct == 0) - delete this; -#endif - - } - - void Referenced::Acquire() const - { -#ifdef LOG_LIFECYCLE_EVENTS - CLog::Log(LOGDEBUG,"REFCNT incrementing to %ld on 0x%lx", - InterlockedIncrement((long*)&refs), (long)(((void*)this))); -#else - AtomicIncrement((long*)&refs); -#endif - } - -#ifdef DEBUG_REFS - void Referenced::check() const - { - long val = refs; - if (val <= 0) - { - CLog::Log(LOGERROR,"REFCNT: access of deleted Referenced. Count is %ld on 0x%lx", - val, (long)(((void*)this))); - } - } -#endif -} diff --git a/xbmc/utils/ReferenceCounting.h b/xbmc/utils/ReferenceCounting.h deleted file mode 100644 index 734a7b2cc7..0000000000 --- a/xbmc/utils/ReferenceCounting.h +++ /dev/null @@ -1,107 +0,0 @@ -/* - * Copyright (C) 2005-2011 Team XBMC - * http://www.xbmc.org - * - * This Program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2, or (at your option) - * any later version. - * - * This Program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with XBMC; see the file COPYING. If not, write to - * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA. - * http://www.gnu.org/copyleft/gpl.html - * - */ - -#pragma once - -#include - -// Comment in this #define in order to log REFCNT errors. -// WARNING! defining DEBUG_REFS prevents the Referenced objects from EVER -// being deleted. They will ALL LEAK! This is just for debugging. -//#define DEBUG_REFS -#ifdef DEBUG_REFS -#define refcheck if (ac != NULL) ac->Referenced::check() -#else -#define refcheck -#endif - -namespace xbmcutil -{ - /** - * This class is the superclass for all reference counted classes. - */ - class Referenced - { - private: - long refs; - - public: - Referenced() : refs(0) {} - virtual ~Referenced(); - - void Release() const; - void Acquire() const; - -#ifdef DEBUG_REFS - void check() const; -#endif - - /** - * This class is a smart pointer for a Referenced class. - */ - template class ref - { - T * ac; - public: - typedef T*(*factory)(); - - inline ref(factory factoryMethod) : ac( (*factoryMethod)() ) { if (ac) ac->Acquire(); refcheck; } - inline ref() : ac(NULL) {} - inline ref(const T* _ac) : ac((T*)_ac) { if (ac) ac->Acquire(); refcheck; } - - // copy semantics - inline ref(ref const & oref) : ac((T*)(oref.get())) { if (ac) ac->Acquire(); refcheck; } - template inline ref(ref const & oref) : ac(static_cast(oref.get())) { if (ac) ac->Acquire(); refcheck; } - - /** - * operator= should work with either another smart pointer or a pointer since it will - * be able to convert a pointer to a smart pointer using one of the above constuctors. - * - * Note: Operator= is ambiguous if you define both an operator=(ref&) and an operator=(T*). I'm - * opting for the route that boost took here figuring it has more history behind it. - */ - inline ref& operator=(ref const & oref) - { if (this != &oref) { if (ac) ac->Release(); ac=((T*)oref.get()); if (ac) ac->Acquire(); } refcheck; return *this; } - - inline T* operator->() const { refcheck; return ac; } - - /** - * This operator doubles as the value in a boolean expression. - */ - inline operator T*() const { refcheck; return ac; } - inline T* get() const { refcheck; return ac; } - inline T& getRef() const { refcheck; return *ac; } - - inline ~ref() { refcheck; if (ac) ac->Release(); } - inline bool isNull() const { refcheck; return ac == NULL; } - inline bool isNotNull() const { refcheck; return ac; } - inline bool isSet() const { refcheck; return ac; } - inline bool operator!() const { refcheck; return ac == NULL; } - - // This is there only for boost compatibility - template inline void reset(ref const & oref) { refcheck; (*this) = static_cast(oref.get()); refcheck; } - template inline void reset(O * oref) { refcheck; (*this) = static_cast(oref); refcheck; } - inline void reset() { refcheck; if (ac) ac->Release(); ac = NULL; } - }; - - }; - -} diff --git a/xbmc/utils/log.h b/xbmc/utils/log.h index aa980f213a..673882ec0b 100644 --- a/xbmc/utils/log.h +++ b/xbmc/utils/log.h @@ -54,7 +54,7 @@ class CLog { public: - class CLogGlobals : public xbmcutil::Referenced + class CLogGlobals { public: CLogGlobals() : m_file(NULL), m_repeatCount(0), m_repeatLogLevel(-1), m_logLevel(LOG_LEVEL_DEBUG) {} diff --git a/xbmc/windowing/X11/WinSystemX11GL.h b/xbmc/windowing/X11/WinSystemX11GL.h index 2864cf8744..a17d356dac 100644 --- a/xbmc/windowing/X11/WinSystemX11GL.h +++ b/xbmc/windowing/X11/WinSystemX11GL.h @@ -27,7 +27,7 @@ #include "rendering/gl/RenderSystemGL.h" #include "utils/GlobalsHandling.h" -class CWinSystemX11GL : public CWinSystemX11, public CRenderSystemGL, public virtual xbmcutil::Referenced +class CWinSystemX11GL : public CWinSystemX11, public CRenderSystemGL { public: CWinSystemX11GL(); diff --git a/xbmc/windowing/egl/WinSystemEGL.h b/xbmc/windowing/egl/WinSystemEGL.h index bb0bcb64a7..0e11a01faf 100644 --- a/xbmc/windowing/egl/WinSystemEGL.h +++ b/xbmc/windowing/egl/WinSystemEGL.h @@ -29,7 +29,7 @@ #include "rendering/gles/RenderSystemGLES.h" #include "utils/GlobalsHandling.h" -class CWinSystemEGL : public CWinSystemBase, public CRenderSystemGLES, public virtual xbmcutil::Referenced +class CWinSystemEGL : public CWinSystemBase, public CRenderSystemGLES { public: CWinSystemEGL(); diff --git a/xbmc/windowing/osx/WinSystemOSXGL.h b/xbmc/windowing/osx/WinSystemOSXGL.h index 9001634a28..d8d8d706e4 100644 --- a/xbmc/windowing/osx/WinSystemOSXGL.h +++ b/xbmc/windowing/osx/WinSystemOSXGL.h @@ -27,7 +27,7 @@ #include "rendering/gl/RenderSystemGL.h" #include "utils/GlobalsHandling.h" -class CWinSystemOSXGL : public CWinSystemOSX, public CRenderSystemGL, public virtual xbmcutil::Referenced +class CWinSystemOSXGL : public CWinSystemOSX, public CRenderSystemGL { public: CWinSystemOSXGL(); diff --git a/xbmc/windowing/windows/WinSystemWin32DX.h b/xbmc/windowing/windows/WinSystemWin32DX.h index 7ee4984979..78b2653760 100644 --- a/xbmc/windowing/windows/WinSystemWin32DX.h +++ b/xbmc/windowing/windows/WinSystemWin32DX.h @@ -33,7 +33,7 @@ #ifdef HAS_DX -class CWinSystemWin32DX : public CWinSystemWin32, public CRenderSystemDX, public virtual xbmcutil::Referenced +class CWinSystemWin32DX : public CWinSystemWin32, public CRenderSystemDX { public: CWinSystemWin32DX(); diff --git a/xbmc/windowing/windows/WinSystemWin32GL.h b/xbmc/windowing/windows/WinSystemWin32GL.h index e13251a586..c356ceb08a 100644 --- a/xbmc/windowing/windows/WinSystemWin32GL.h +++ b/xbmc/windowing/windows/WinSystemWin32GL.h @@ -32,7 +32,7 @@ #include "rendering/gl/RenderSystemGL.h" #include "utils/GlobalsHandling.h" -class CWinSystemWin32GL : public CWinSystemWin32, public CRenderSystemGL, public virtual xbmcutil::Referenced +class CWinSystemWin32GL : public CWinSystemWin32, public CRenderSystemGL { public: CWinSystemWin32GL();