diff --git a/core/ConCmdManager.cpp b/core/ConCmdManager.cpp index 80d3f703..e93c6cc3 100644 --- a/core/ConCmdManager.cpp +++ b/core/ConCmdManager.cpp @@ -383,7 +383,7 @@ bool ConCmdManager::AddAdminCommand(IPluginFunction *pFunction, { if (!m_CmdGrps.add(i, group)) return false; - i->value = NoAddRef(new CommandGroup()); + i->value = new CommandGroup(); } Ref cmdgroup = i->value; diff --git a/core/logic/GameConfigs.cpp b/core/logic/GameConfigs.cpp index 12067853..8c122425 100644 --- a/core/logic/GameConfigs.cpp +++ b/core/logic/GameConfigs.cpp @@ -1082,6 +1082,7 @@ bool GameConfigManager::LoadGameConfigFile(const char *file, IGameConfig **_pCon } pConfig = new CGameConfig(file); + pConfig->AddRef(); /* :HACKHACK: Don't parse the main config file */ bool retval = true; diff --git a/core/logic/ShareSys.cpp b/core/logic/ShareSys.cpp index 77769662..ddde22a5 100644 --- a/core/logic/ShareSys.cpp +++ b/core/logic/ShareSys.cpp @@ -375,7 +375,7 @@ PassRef ShareSystem::AddNativeToCache(CNativeOwner *pOwner, const sp_nat if (i.found()) return NULL; - Ref entry = Newborn(new Native(pOwner, ntv)); + Ref entry = new Native(pOwner, ntv); m_NtvCache.insert(ntv->name, entry); return entry; } @@ -415,7 +415,7 @@ PassRef ShareSystem::AddFakeNative(IPluginFunction *pFunc, const char *n CNativeOwner *owner = g_PluginSys.GetPluginByCtx(fake->ctx->GetContext()); - entry = Newborn(new Native(owner, fake.take())); + entry = new Native(owner, fake.take()); m_NtvCache.insert(name, entry); return entry; diff --git a/core/logic/smn_adt_trie.cpp b/core/logic/smn_adt_trie.cpp index ab70f8cc..989df0f4 100644 --- a/core/logic/smn_adt_trie.cpp +++ b/core/logic/smn_adt_trie.cpp @@ -163,7 +163,7 @@ private: cell_t data_; }; -struct CellTrie : public ke::Refcounted +struct CellTrie { StringHashMap map; }; @@ -204,8 +204,7 @@ public: //IHandleTypeDispatch { if (type == htCellTrie) { - CellTrie *pTrie = (CellTrie *)object; - pTrie->Release(); + delete (CellTrie *)object; } else { TrieSnapshot *snapshot = (TrieSnapshot *)object; delete snapshot; diff --git a/extensions/clientprefs/extension.cpp b/extensions/clientprefs/extension.cpp index 96a3173b..7ff5eee2 100644 --- a/extensions/clientprefs/extension.cpp +++ b/extensions/clientprefs/extension.cpp @@ -219,7 +219,7 @@ void ClientPrefs::DatabaseConnect() char error[256]; int errCode = 0; - Database = Newborn(Driver->Connect(DBInfo, true, error, sizeof(error))); + Database = AdoptRef(Driver->Connect(DBInfo, true, error, sizeof(error))); if (!Database) { diff --git a/public/amtl/am-refcounting-threadsafe.h b/public/amtl/am-refcounting-threadsafe.h index a37b39d4..785ba62b 100644 --- a/public/amtl/am-refcounting-threadsafe.h +++ b/public/amtl/am-refcounting-threadsafe.h @@ -35,12 +35,15 @@ namespace ke { +// See the comment above Refcounted for more information. This class is +// identical, except changing the reference count is guaranteed to be atomic +// with respect to other threads changing the reference count. template class RefcountedThreadsafe { public: RefcountedThreadsafe() - : refcount_(1) + : refcount_(0) { } diff --git a/public/amtl/am-refcounting.h b/public/amtl/am-refcounting.h index 71a6f554..028d6df5 100644 --- a/public/amtl/am-refcounting.h +++ b/public/amtl/am-refcounting.h @@ -37,18 +37,28 @@ namespace ke { template class Ref; -// Holds a refcounted T without addrefing it. This is similar to PassRef<> -// below, but is intended only for freshly allocated objects which start -// with reference count 1, and we don't want to add an extra ref just by -// assigning to PassRef<> or Ref<>. +// Objects in AMTL inheriting from Refcounted will have an initial refcount +// of 0. However, in some systems (such as COM), the initial refcount is 1, +// or functions may return raw pointers that have been AddRef'd. In these +// cases it would be a mistake to use Ref<> or PassRef<>, since the object +// would leak an extra reference. +// +// This container holds a refcounted object without addrefing it. This is +// intended only for interacting with functions which return an object that +// has been manually AddRef'd. Note that this will perform a Release(), so +// so it is necessary to assign it to retain the object. template -class Newborn +class AlreadyRefed { public: - Newborn(T *t) + AlreadyRefed(T *t) : thing_(t) { } + ~AlreadyRefed() { + if (thing_) + thing_->Release(); + } T *release() const { return ReturnAndVoid(thing_); @@ -59,10 +69,10 @@ class Newborn }; template -static inline Newborn -NoAddRef(T *t) +static inline AlreadyRefed +AdoptRef(T *t) { - return Newborn(t); + return AlreadyRefed(t); } // When returning a value, we'd rather not be needlessly changing the refcount, @@ -81,7 +91,14 @@ class PassRef { } - PassRef(const Newborn &other) + PassRef(const AlreadyRefed &other) + : thing_(other.release()) + { + // Don't addref, newborn means already addref'd. + } + + template + PassRef(const AlreadyRefed &other) : thing_(other.release()) { // Don't addref, newborn means already addref'd. @@ -134,7 +151,7 @@ class PassRef private: // Disallowed operators. PassRef &operator =(T *other); - PassRef &operator =(Newborn &other); + PassRef &operator =(AlreadyRefed &other); void AddRef() { if (thing_) @@ -149,13 +166,18 @@ class PassRef mutable T *thing_; }; -// Classes which are refcounted should inherit from this. +// Classes which are refcounted should inherit from this. Note that reference +// counts start at 0 in AMTL, rather than 1. This avoids the complexity of +// having to adopt the initial ref upon allocation. However, this also means +// invoking Release() on a newly allocated object is illegal. Newborn objects +// must either be assigned to a Ref or PassRef (NOT an AdoptRef/AlreadyRefed), +// or must be deleted using |delete|. template class Refcounted { public: Refcounted() - : refcount_(1) + : refcount_(0) { } @@ -217,7 +239,12 @@ class Ref : thing_(other.release()) { } - Ref(const Newborn &other) + Ref(const AlreadyRefed &other) + : thing_(other.release()) + { + } + template + Ref(const AlreadyRefed &other) : thing_(other.release()) { } @@ -255,7 +282,7 @@ class Ref } template - Ref &operator =(const Newborn &other) { + Ref &operator =(const AlreadyRefed &other) { Release(); thing_ = other.release(); return *this;