Thread: Smart pointer class

  1. #1
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654

    Smart pointer class

    Code:
    #pragma once
    #pragma warning ( disable: 4312 )
    
    class CMemoryManagerNull;
    template<typename T> class CMemoryManagerNew;
    
    // Requires MFC.
    // Requires afxmt.h
    
    template<typename T> class CMemoryManager
    {
    protected:
    #define MM_ERROR(x, y) try { x; } catch(char*) { y; }
    #define MM_ERROR_BLOCK(x) try { x; } catch(char*)
    	struct PointerInfo
    	{
    		DWORD dwRefCount;
    		CCriticalSection cSync;
    		bool bArray;
    		bool bSetIfArray;
    		bool bExistInMap;
    	};
    	struct PointerInfoTemp
    	{
    		PointerInfo* m_pTempInfo;
    		void* pForAddr;
    		bool bLookedUp;
    	};
    
    	T* p;
    	PointerInfo* m_pInfo;
    	//PointerInfoTemp m_PTempInfoLocal; // 
    	PointerInfoTemp m_PTempInfo;
    	DWORD m_dwThreadId;
    	bool m_bThreadSafe;
    	bool m_bTimeCritical;
    	
    	static CMap<const void*, const void*, PointerInfo*, PointerInfo*> m_InfoMap;
    	static CCriticalSection cThisSync;
    
    	void Init()
    	{
    		p = NULL;
    		m_pInfo = NULL;
    	}
    	void FirstInit(bool bThreadSafe = false, bool bTimeCritical = false)
    	{
    		m_bTimeCritical = bTimeCritical;
    		m_bThreadSafe = bThreadSafe;
    		m_dwThreadId = GetCurrentThreadId();
    		m_PTempInfo.m_pTempInfo = NULL;
    		m_PTempInfo.pForAddr = NULL;
    		m_PTempInfo.bLookedUp = false;
    	}
    	void CheckPointerInfo()
    	{
    		if (m_pInfo == NULL) 
    		{
    			m_pInfo = new PointerInfo;
    			m_pInfo->bArray = false;
    			m_pInfo->bSetIfArray = false;
    			m_pInfo->bExistInMap = false;
    			m_pInfo->dwRefCount = 0;
    		}
    	}
    	bool LookupSetRefCount(const T* p)
    	{
    		ASSERT(!m_bTimeCritical);
    		if ( !m_PTempInfo.bLookedUp || (m_PTempInfo.bLookedUp && m_PTempInfo.pForAddr != p) )
    		{
    			CMemoryManager::cThisSync.Lock();
    			CMemoryManager::m_InfoMap.Lookup(p, m_PTempInfo.m_pTempInfo);
    			CMemoryManager::cThisSync.Unlock();
    			if (m_PTempInfo.m_pTempInfo)
    			{
    				m_PTempInfo.pForAddr = (T*)p;
    				m_PTempInfo.bLookedUp = true;
    			}
    		}
    		if (m_PTempInfo.m_pTempInfo)
    		{
    			delete m_pInfo;
    			m_pInfo = m_PTempInfo.m_pTempInfo;
    			IncreaseRefCount();
    			return true;
    		}
    		return false;
    	}
    	void SetRefCount(DWORD dwNewCount) const
    	{
    		CheckPointerInfo();
    		if (!m_bTimeCritical) Verify();
    		Lock();
    		*m_pInfo->pdwRefCount = dwNewCount;
    		Unlock();
    	}
    	void IncreaseRefCount()
    	{
    		CheckPointerInfo();
    		if (!m_bTimeCritical) Verify();
    		Lock();
    		m_pInfo->dwRefCount++;
    		Unlock();
    	}
    	void TraceErrorAndDelete(CMemoryException* pException, CString strCallFunction, CString strErrFunction, const T* pMemory, bool bArray)
    	{
    		CString strError;
    		pException->GetErrorMessage(strError.GetBuffer(100000), 100000);
    		strError.ReleaseBuffer();
    		TRACE("&#37;s: Exception in %s:\n%s\nUnable to complete.", strCallFunction, strErrFunction, strError);
    		pException->Delete();
    		if (bArray)
    			delete [] pMemory;
    		else
    			delete pMemory;
    		ASSERT(FALSE);
    	}
    	void SetNewPointer(T* pNew, bool bArray)
    	{
    		if (p == pNew) return; // SPECIAL CASE: If we assign a pointer to the class that the class is already handling, the stop here. The chances are that if we proceed, it will decrement the reference 
    							   // count of the memory and delete it because it reaches 0. Thus, the memory we're trying to assign also gets freed! To avoid this, we do a return here.
    		InternalDelete();
    		if (!m_bTimeCritical) 
    		{
    			try
    			{
    				if ( LookupSetRefCount(pNew) )
    					ASSERT(m_pInfo->bArray == bArray); // Make sure the calle actually passed the right value for bArray; otherwise there's a programming error or the incorrect memory found!
    			}
    			catch(CMemoryException* pException)
    			{
    				TraceErrorAndDelete(pException, _T("CMemoryManager::SetNewPointer"), _T("CMemoryManager::LookupSetRefCount"), pNew, bArray);
    				return;
    			}
    		}
    		try
    		{
    			IncreaseRefCount();
    		}
    		catch(CMemoryException* pException)
    		{
    			TraceErrorAndDelete(pException, _T("CMemoryManager::SetNewPointer"), _T("CMemoryManager::IncreaseRefCount"), pNew, bArray);
    			return;
    		}
    		p = pNew;
    		m_pInfo->bArray = bArray;
    		if (!m_bTimeCritical) SetPointerInMap();
    		if (!m_bTimeCritical) Verify();
    	}
    	void InternalDelete()
    	{
    		InternalDetach();
    		Init();
    	}
    	void SetPointerInMap() const
    	{
    		ASSERT(!m_bTimeCritical);
    		if (!m_pInfo->bExistInMap)
    		{
    			CMemoryManager::cThisSync.Lock();
    			CMemoryManager::m_InfoMap.SetAt(p, m_pInfo);
    			CMemoryManager::cThisSync.Unlock();
    			m_pInfo->bExistInMap = true;
    		}
    	}
    	void DeletePointerInMap() const
    	{
    		ASSERT(!m_bTimeCritical);
    		if (m_pInfo->bExistInMap)
    		{
    			CMemoryManager::cThisSync.Lock();
    			CMemoryManager::m_InfoMap.RemoveKey(p);
    			CMemoryManager::cThisSync.Unlock();
    			m_pInfo->bExistInMap = false;
    		}
    	}
    	void UpdateMemoryManager(const CMemoryManager& rmm)
    	{
    		InternalDelete();
    		if (rmm.p == NULL) return;
    		p = rmm.p;
    		m_pInfo = rmm.m_pInfo;
    		IncreaseRefCount();
    		if (!m_bTimeCritical) Verify();
    	}
    	void SetNewAddr(T* pNewAddr, DWORD dwRefCount) const
    	{
    		Delete();
    		SetNewPointer(pNewAddr);
    		SetRefCount(dwRefCount);
    		if (!m_bTimeCritical) Verify();
    	}
    	void InternalDetach()
    	{
    		if (m_pInfo == NULL) return;
    		ASSERT(!m_pInfo->bSetIfArray); // This will assert if the class was unable to determine if an array was assigned and the caller hasn't called SetIfArray()
    		if (m_pInfo->dwRefCount - 1 <= 1) 
    		{
    			Lock();
    			if (!m_bTimeCritical) DeletePointerInMap();
    			if (m_pInfo->bArray)
    				delete [] p;
    			else
    				delete p;
    			delete m_pInfo;
    			Unlock();
    			Init();
    		}
    		else
    		{
    			Lock();
    			m_pInfo->dwRefCount--;
    			Unlock();
    		}
    	}
    	void Verify() const
    	{
    #ifdef _DEBUG
    		//ASSERT(p != NULL);
    		ASSERT(!m_bTimeCritical);
    		ASSERT(m_pInfo != NULL);
    #endif
    	}
    	bool IsMemoryArray(const T* p)
    	{
    		ASSERT(!m_bTimeCritical);
    		if ( !m_PTempInfo.bLookedUp || (m_PTempInfo.bLookedUp && m_PTempInfo.pForAddr != p) )
    		{
    			CMemoryManager::cThisSync.Lock();
    			CMemoryManager::m_InfoMap.Lookup(p, m_PTempInfo.m_pTempInfo);
    			CMemoryManager::cThisSync.Unlock();
    			m_PTempInfo.pForAddr = (T*)p;
    			m_PTempInfo.bLookedUp = true;
    		}
    		//CMemoryManager::m_InfoMap.Lookup(p, pInfo);
    		if (m_PTempInfo.m_pTempInfo)
    			return m_PTempInfo.m_pTempInfo->bArray;
    		else
    			throw "m_PTempInfo.m_pTempInfo == NULL";
    		return false;
    	}
    	void OperatorAssign(T* pmm)
    	{
    		bool bArray = false;
    		bool bSetIfArray = false;
    		bool bGuessIfArray = false;
    		if (!m_bTimeCritical)
    			MM_ERROR_BLOCK( bArray = IsMemoryArray(pmm) ) { bGuessIfArray = true; }
    		else
    		{
    			TRACE("\nCMemoryManager::void operator = (const T*& pmm):\nWARNING: Time Critical mode enabled. No check will me made to see if pointer is an array.\nAttempting to guess.");
    			bGuessIfArray = true;
    		}		
    		if (bGuessIfArray)
    		{
    			TRACE("\nCMemoryManager::void operator = (const T*& pmm):\nWARNING: Unable to determine if pointer is array or not. Attempting to guess.\n");
    			size_t nSize = _msize((void*)pmm);
    			if ( nSize > sizeof(*pmm) )
    			{
    				if (nSize % sizeof(*pmm) == 0) // Size of memory block is greater than sizeof the pointer and can be divided evenly. That probably means there are more than 1 element.
    					TRACE("NOTICE: Size of memory block pointed to by pmm was greater than the size of the data type, but evenly dividable by the data size. Pointer is assumed to be an array.\n");
    				else
    				{
    					TRACE("WARNING: Size of memory block pointed to by pmm was greater than the size of the data type, but was NOT evenly dividable by the data size. Pointer may or may not be an array. A call to SetIfArray() is required.\n");
    					bSetIfArray = true;
    				}
    				bArray = true;
    			}
    			else
    			{
    				TRACE("NOTICE: Size of memory block pointed to by pmm was equal to the size of the data type. No array was detected.\n");
    				bArray = false;
    			}
    			TRACE("\n");
    		}
    		SetNewPointer(pmm, bArray);
    		if (bSetIfArray) m_pInfo->bSetIfArray = true;
    		if (!m_bTimeCritical) Verify();
    	}
    
    public:
    	CMemoryManager()
    	{
    		FirstInit();
    		Init();
    	}
    	explicit CMemoryManager(bool bThreadSafe, bool bTimeCritical)
    	{
    		FirstInit(bThreadSafe, bTimeCritical);
    		Init();
    	}
    	CMemoryManager(int/* nNull*/)
    	{
    		FirstInit();
    		Init();
    	}
    	//CMemoryManager(const CMemoryManager<T>& rmm)
    	//{
    	//	FirstInit();
    	//	Init();
    	//	UpdateMemoryManager(rmm);
    	//}
    	//CMemoryManager(CMemoryManagerNull*& rNull)
    	//{
    	//	FirstInit();
    	//	delete rNull;
    	//	Init();
    	//}
    	//CMemoryManager(const T* p)
    	//{
    	//	bThreadSafe = true;
    	//	dwThreadId = GetCurrentThreadId();
    	//	Init();
    	//	OperatorAssign(p);
    	//}
    	~CMemoryManager()
    	{
    		InternalDetach();
    	}
    	void Lock() const
    	{
    #ifdef _DEBUG
    		ASSERT(GetCurrentThreadId() == m_dwThreadId); // The class should only be used by one thread only - and that's the thread that created it.
    #endif
    		if (p == NULL) return;
    		if (m_bThreadSafe) m_pInfo->cSync.Lock();
    	}
    	void Unlock() const
    	{
    		ASSERT(GetCurrentThreadId() == m_dwThreadId); // The class should only be used by one thread only - and that's the thread that created it.
    		if (p == NULL) return;
    		if (m_bThreadSafe) m_pInfo->cSync.Unlock();
    	}
    	void SetIfArray(bool bArray) const
    	{
    		ASSERT(bSetIfArray); // Should only be called if user is required to set if it's an array or not.
    		if (!bSetIfArray) return;
    		CheckPointerInfo();
    		m_pInfo->bArray = bArray;
    		bSetIfArray = false;
    	}
    	void Release()
    	{
    		InternalDelete();
    	}
    	// These functions are synonyms for Release, to be used to make code more readable.
    	void RemoveRef() const { Delete(); }
    	// Memory handled by this class is freed / allocated automatically, as therefore, it is not allowed to tamper with the memory pointer by setting it to NULL for example.
    	// The only allowed things to do are update the pointer and free the memory upon which it is set to NULL automatically.
    	void operator = (int Val) const { ASSERT(Val == NULL); Delete(); Verify(); }
    	bool operator == (int nCompare) const { return (p == (T*)nCompare); }
    	void operator = (T* pmm)
    	{
    		OperatorAssign(pmm);
    	}
    	//void operator = (T* pmm)
    	//{
    	//	*this = (const T*)pmm;
    	//}
    	void operator = (const CMemoryManager& rmm)
    	{
    		UpdateMemoryManager(rmm);
    		//Verify();
    	}
    	//void operator = (const CMemoryManagerNull* pNull)
    	//{
    	//	delete pNull;
    	//	Delete();
    	//}
    	//void AttachNewPointer(T* pNew)
    	//{
    	//	// If you have done something with the memory that renders the old pointer invalid (like reallocating), then use this function to set the DEBUG_NEW pointer.
    	//	// DO NOT USE OTHERWISE. This function does not perform any checks, modify the reference count, or anything else. If you need to assign a DEBUG_NEW pointer, use the assigment
    	//	// operator.
    	//	p = pNew;
    	//	Verify();
    	//}
    
    	void New(bool bArray = false, DWORD dwElements = 1)
    	{
    		InternalDelete();
    		T* pMemory;
    		if (bArray)
    			pMemory = new T[dwElements];
    		else
    			pMemory = new T;
    		SetNewPointer(pMemory, bArray);
    		if (!m_bTimeCritical) Verify();
    	}
    	void Attach(T*& pToAttach, bool bArray = false/*, DWORD dwRefCount = 1*/)
    	{
    		// If you got some memory you want the class to manage (example: data passed by thread you want the thread to clean up), then this is the function for you!
    		// PLEASE NOTE: Original pointer will be NULL after this function! Do not use or manipulate the memory once you've attached it to the class (hence why the original pointer will be NULL)!
    		// 
    		//SetNewPointer(pToAttach, bArray);
    		bArray; // bArray is kept for backwards compability
    		*this = pToAttach;
    		pToAttach = NULL;
    	}
    
    	template<typename T> static CMemoryManager<T> MemoryManagerNew(bool bArray, T* pNew)
    	{
    		return MemoryManagerNew(pNew, bArray);
    	}
    	template<typename T> static CMemoryManager<T> MemoryManagerNew(T* pNew, bool bArray)
    	{
    		CMemoryManager<T> mm;
    		T* pLocal = pNew;
    		mm.Attach(pLocal, bArray);
    		return mm;
    	}
    
    	// Used to cast or transform a memory manager of type TranformFrom to TranformTo. Use with caution. Primarily for use with polymorphism.
    	//template<typename TranformFrom, typename TranformTo> static CMemoryManager<TranformTo> MemoryManagerTansform(CMemoryManager<TranformFrom> pTransformFrom)
    	//{
    	//	TranformFrom* pTransform = (TranformFrom*)pTransformFrom;
    	//	TranformTo* pTransformTo = dynamic_cast<TranformTo>(pTransform); // Security check to make sure it's safe to transform from T1 to T2
    	//	CMemoryManager<TransformTo> pTransformToB;
    	//	pTransformToB.p = pTransformTo;
    	//	pTransformToB.pdwRefCount = pTransformFrom.pdwRefCount;
    	//	pTransformToB.IncreaseRefCount();
    	//	pTransformToB.bArray = pTransformFrom.bArray;
    	//	//pTransformToB.Attach(pTransformFrom.IsArray(), &pTransform, pTransformFrom.GetReferenceCount() + 1);
    	//	return pTransformTo;
    	//}
    
    	//void SetTimeCritical(bool bTimeCritical) { m_bTimeCritical = bTimeCritical; }
    	//void SetThreadSafe(bool bThreadSafe) { m_bThreadSafe = bThreadSafe; }
    	int GetSize() const { ASSERT(p); return sizeof(*p); }
    	size_t GetTotalSize() const { Lock(); size_t nSize = _msize(p); Unlock(); return nSize; }
    	int GetElements() const { return GetTotalSize() / sizeof(*p); }
    	void Resize(size_t dwNewSize) const { Lock(); realloc(p, dwNewSize); Unlock(); }
    	void ResizeElements(DWORD dwNewSize) const { Lock(); realloc(p, sizeof(*p) * dwNewSize); Unlock(); }
    	bool IsArray() const { return bArray; }
    	DWORD GetRefCount()  const
    	{ 
    		Lock();
    		DWORD dwToReturn = 0;
    		if (pdwRefCount) dwToReturn = *pdwRefCount;
    		Unlock();
    		return dwToReturn;
    	}
    
    	T& operator * () const { Lock(); T& r = *p; Unlock(); return r; }
    	T& operator * (CMemoryManagerNew<T>& Value) const { ASSERT(p); Lock(); *p = Value; Unlock(); }
    	T* operator -> () const { ASSERT(p); return p; }
    	operator T* () const { return p; }
    	operator void* () const { return p; }
    	//operator CMemoryManager<T>* () { return this; }
    	operator T& () const { return *p; }
    	T* GetPointer() const { return p; } 
    	bool operator == (T* pCompare) const { ASSERT(p); Lock(); bool b = (*p == *pCompare); Unlock(); return b; }
    	bool operator == (const CMemoryManager<T>& rCompare) const { ASSERT(p); Lock(); bool b = (*p == *rCompare.p); Unlock(); return b; }
    	bool operator == (const CMemoryManager<T>* pCompare) const { ASSERT(p); Lock(); bool b = (*p == pCompare->p); Unlock(); return b; }
    	bool operator == (const CMemoryManagerNull&) const { return (p == NULL); }
    	bool operator == (const CMemoryManagerNull* pNull) const { Lock(); delete pNull; return (p == NULL); Unlock(); }
    	//bool operator != (int nCompare) { return (p != (T*)nCompare); }
    	bool operator != (T* pCompare) const { ASSERT(p); Lock(); bool b = (*p != *pCompare); Unlock(); return b; }
    	bool operator != (const CMemoryManager<T>& rCompare) const { ASSERT(p); Lock(); bool b = (*p != *rCompare.p); Unlock(); return b; }
    	bool operator != (const CMemoryManager<T>* pCompare) const { ASSERT(p); Lock(); bool b = (*p != pCompare->p); Unlock(); return b; }
    	//void operator = (CMemoryManagerNull* pNull) { Delete(); Init(); Verify(); delete pNull; }
    	operator bool () const { return p != NULL; }
    	// Restrict operator +? They're dangerous!
    	//T* operator + (UINT64 add) explicit const { return p + add; }
    	//T* operator + (DWORD add) explicit const { return p + add; }
    	//T* operator + (T* add) explicit const { return p + add; }
    	template<typename NewT> operator CMemoryManager<NewT>& () const 
    	{
    		//T* pFrom = p;
    		NewT* pTo = dynamic_cast<NewT*>(p); // Security cast to make sure the new type in compatible with the old
    		ASSERT(pTo);
    		return *(CMemoryManager<NewT>*)this; 
    	}
    	T& operator [] (UINT index) const
    	{
    		Lock();
    		UINT nSize = _msize(p);
    		Unlock();
    		ASSERT(sizeof(p) * index > nSize);
    		T& rReturn = p[index]; 
    		return rReturn;
    	}
    	//T& operator [] (int index) { return p[index]; }
    	//T* P() { return p; }
    	void operator = (CMemoryManagerNull* p) const { delete p; Release(); }
    	//operator CMemoryManagerDefault<T> { return new CMemoryManagerDefault
    
    protected:
    	void operator delete(void* /*p*/) { }
    	void operator delete(void* /*p*/, size_t /*_Count*/) { }
    	//CMemoryManager<T>* operator & ();
    	void* operator new(size_t _Count) throw();
    	void* operator new(size_t _Count, const std::nothrow_t&) throw();
    	void* operator new(size_t _Count, void* _Ptr) throw();
    };
    
    template<typename T> class CMemoryManagerNew: public CMemoryManager<T>
    {
    public:
    	//CMemoryManager(bool bAllocate)
    	//{
    	//	Init();
    	//	if (bAllocate) New();
    	//}
    	CMemoryManagerNew()
    	{
    		//Init();
    		New();
    		Verify();
    	}
    	CMemoryManagerNew(bool bArray, T* pNew)
    	{
    		//Init();
    		SetNewPointer(pNew, bArray);
    		Verify();
    	}
    	CMemoryManagerNew(T NewValue)
    	{
    		//Init();
    		New();
    		Verify();
    		*p = NewValue;
    	}
    	CMemoryManagerNew(bool bThreadSafe, bool bTimeCritical)
    	{
    		FirstInit(bThreadSafe, bTimeCritical);
    		*(CMemoryManager*)this = rpp;
    	}
    	CMemoryManagerNew(bool bArray, T* pNew, bool bThreadSafe, bool bTimeCritical)
    	{
    		FirstInit(bThreadSafe, bTimeCritical);
    		SetNewPointer(pNew, bArray);
    		Verify();
    	}
    	CMemoryManagerNew(T NewValue, bool bThreadSafe, bool bTimeCritical)
    	{
    		FirstInit(bThreadSafe, bTimeCritical);
    		New();
    		Verify();
    		*p = NewValue;
    	}
    };
    
    template<typename T> class CMemoryManagerDefault: public CMemoryManager<T>
    {
    public:
    	CMemoryManagerDefault(CMemoryManagerNull* pNull)
    	{
    		delete pNull;
    		Release();
    	}
    	CMemoryManagerDefault(CMemoryManager<T>& rpp)
    	{
    		*(CMemoryManager*)this = rpp;
    	}
    };
    
    template<typename T> CMap<const void*, const void*, typename CMemoryManager<T>::PointerInfo*, typename CMemoryManager<T>::PointerInfo*> CMemoryManager<T>::m_InfoMap;
    template<typename T> CCriticalSection CMemoryManager<T>::cThisSync;
    
    class CMemoryManagerNull { };
    
    #pragma warning ( default: 4312 )
    #define pp CMemoryManager
    #define ppnew CMemoryManagerNew
    #define ppdef CMemoryManagerDefault
    #define newpp CMemoryManager<CHAR>::MemoryManagerNew
    //#define tpp CMemoryManager<CHAR>::MemoryManagerTansform
    //#define PPNULL new CMemoryManagerNull
    Last edited by Elysia; 10-31-2007 at 10:36 AM.

  2. #2
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    I took about 15 minutes to look at the code, and I have identified:

    1) A certain crash in LookupSetRefCount(): first it deletes m_pInfo and then it calls DeletePointerInfo, which in turn accesses m_pInfo without checking (in order to delete m_pInfo->m_pdwCount) and then deletes m_pInfo again. That's an access to deleted memory and a double delete.

    2) Const correctness issues. At various points you use const T*, for no good reason, and later you have to cast away the constness again.
    Remember this simple rule: if you have to cast away constness for any reason other than a broken legacy API, your code is broken.

    3) Exception safety. If I call New() and the pointer info's CCriticalSection::Lock() throws (it can), the memory allocated by New() is leaked. Worse, if the static lock of the map fails, the local lock is never unlocked! You also leak if allocating the pointer info or the reference count fails.

    4) Various subtle threading issues. Most importantly, your reference count reduction is not guaranteed to be atomic and is thus subject to concurrency failure. Nor is it included under the lock, which can be a problem under obscure circumstances.

    5) Leaks due to interface circumstances that ought to be forbidden. In particular, do this: make the not time critical, point to some memory, then call SetTimeCritical(true) and destruct the pointer. Result: the map entry is leaked. (I predicted such problems, by the way.)

    6) An overloaded interface that tries to do way too much.
    The whole thing about being able to point to both arrays and single objects is a mess. It's unintuitive and brittle. I just can't see it as useful either: I can't think of a function that might accept both an array and a single object (as opposed to a 1-element array). At least not one where I would call the design sane.
    Your map lookup and runtime setting of "fast mode" have already led to the problems I predicted.

    7) A convoluted implementation that you have yourself lost track of. Problem #1 shows this. Another, harmless incarnation of this problem is shown by the completely superfluous InternalDelete(). This little method first calls InternalDetach() and then Init(). What you forgot is that InternalDetach() either returns immediately if m_pInfo is null (which implies that p is null, too, unless you have a really big leak in that code) or calls Init() by itself. Either way, InternalDelete()'s call to Init() is redundant. And because of this, its implementation could be reduced to just calling InternalDetach(), which means it's an alias of this method and should be summarily removed.
    A similarly superfluous method is AssignNewPointer(). In C++ tutorials, this is commonly shown as an example of a bad function:
    Code:
    int add(int a, int b) { return a + b; }
    AssignNewPointer() is just that: a single, simple statement wrapped in a function name.
    Not to mention, the main reason it took me so long to get into this code is that it is so all over the place. You seem to have a lot of redundancy in your implementation, leading to hard to understand code.

    8) Various minor issues.
    Your Realloc() and ReallocElements() methods call realloc() on memory allocated with new. That's a big no-no. Worse, you completely misunderstand the way realloc() works and thus, in addition to invoking undefined behaviour due to memcpying non-PODs, you also leak the entire memory if realloc() has to relocate, and you're left with a dangling pointer, too.
    Operator + is indeed dangerous and best removed.
    Allocating a new, special object just to assign null to a pointer? WTF??? Not to mention that people might be tempted to compare pointers against PPNULL (It makes sense. After p = PPNULL; it should be expected that p == PPNULL. Not only is the comparison false, it also leaks memory.)
    Why is the reference count not directly embedded in the PointerInfo struct? Why allocate it separately? Doing that just exposes you to problems.
    What's this TempPointerInfo? Why the name? An hour has passed now since I started looking at your post, and still I don't really have an idea.


    Whoa, an hour! I certainly spent a lot of time on this. But after the way I keep criticizing your ideas, it seemed fair that I actually look at what you produced.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  3. #3
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Holy... Cow. I'm not sure I get the point of any of this. Not a criticism, just an honest question: what the heck is your objective here?

  4. #4
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Well I don't really have time to read the whole thing (plus my brain is melting from looking at really bad C code all day), but if you're trying to create something like an auto_ptr class, I wrote my own a few years ago. You can check how it works and maybe get some ideas (I basically stole, err, I mean borrowed most of it from the STL auto_ptr class):
    http://tech.groups.yahoo.com/group/boost/files/AutoPtr/

    There's also the cases where the compiler converts and uses faulty code in the smart pointer. Having a constructor that takes a single bool, you could actually assign an integer to the class and the constructor will construct a temporary class and call the bool constructor and then assigning it, which is plainly wrong. I want to avoid such mishaps, as well.
    That's exactly what the explicit keyword is for.
    If you look at my AutoPtr constructors, you'll see that I declared them as explicit to prevent automatic type conversions.

  5. #5
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    It may look poor to you because I'm pretty much self-learned
    Mostly I haven't tested much of the new implemtations, though so far it has worked.
    But as I did mention, I'm here to learn
    I'll have a look at those comments and change the class accordingly.

    I might also take a look at that autoptr cpjust!

  6. #6
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Quote Originally Posted by Elysia View Post
    It may look poor to you because I'm pretty much self-learned
    I'm not saying it looks poor. I'm just not sure what you plan on doing with it.

  7. #7
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by CornedBee View Post
    1) A certain crash in LookupSetRefCount(): first it deletes m_pInfo and then it calls DeletePointerInfo, which in turn accesses m_pInfo without checking (in order to delete m_pInfo->m_pdwCount) and then deletes m_pInfo again. That's an access to deleted memory and a double delete.
    You're right. LookupSetRefCount() should not delete m_pInfo; DeletePointerInfo() should. Fixed.

    2) Const correctness issues. At various points you use const T*, for no good reason, and later you have to cast away the constness again.
    Remember this simple rule: if you have to cast away constness for any reason other than a broken legacy API, your code is broken.
    This is my thought too. It should always be const unless I have to do away with const. The only exceptions are certain functions that does not modify the pointer and does not take a const pointer. I'm going to look over the code to see if I can find any, but you can always help me point out what you found.

    3) Exception safety. If I call New() and the pointer info's CCriticalSection::Lock() throws (it can), the memory allocated by New() is leaked. Worse, if the static lock of the map fails, the local lock is never unlocked! You also leak if allocating the pointer info or the reference count fails.
    I added checks to catch CMemoryException* if a Lock() fails in SetNewPointer. If it fails, newly allocated is deleted and the function fails with an ASSERT (also prints a TRACE with error info).

    4) Various subtle threading issues. Most importantly, your reference count reduction is not guaranteed to be atomic and is thus subject to concurrency failure. Nor is it included under the lock, which can be a problem under obscure circumstances.
    Of course, bug. Now it should be fixed. I don't know if I should require atomic and use InterlockedIncrement/Decrement. The stupid functions require a signed LONG. Locks should be enough, or should it?

    5) Leaks due to interface circumstances that ought to be forbidden. In particular, do this: make the not time critical, point to some memory, then call SetTimeCritical(true) and destruct the pointer. Result: the map entry is leaked. (I predicted such problems, by the way.)
    Initially, I used the two functions because there was no constructor to do so. Now I've disabled those functions: objects must be made thread safe or time critical through a call to the correct constructor.

    6) An overloaded interface that tries to do way too much.
    The whole thing about being able to point to both arrays and single objects is a mess. It's unintuitive and brittle. I just can't see it as useful either: I can't think of a function that might accept both an array and a single object (as opposed to a 1-element array). At least not one where I would call the design sane.
    Your map lookup and runtime setting of "fast mode" have already led to the problems I predicted.
    I don't find the design bloated at all. It provides common access to functions you might use with pointers. What exactly do you find bad?
    And the class should NEVER EVER point to a local variable. It should always point to an object on the heap. At first, I only allowed memory created through the class to be used with it, but then I realized I had to have an Attach function to attach memory pointers when I had to handle memory and the pointer came from somewhere that did not use the class.

    7) A convoluted implementation that you have yourself lost track of. Problem #1 shows this.
    Meaning?

    Another, harmless incarnation of this problem is shown by the completely superfluous InternalDelete(). This little method first calls InternalDetach() and then Init(). What you forgot is that InternalDetach() either returns immediately if m_pInfo is null (which implies that p is null, too, unless you have a really big leak in that code) or calls Init() by itself. Either way, InternalDelete()'s call to Init() is redundant. And because of this, its implementation could be reduced to just calling InternalDetach(), which means it's an alias of this method and should be summarily removed.
    Quite. I may look at trying to remove superflous function calls. Though they do not harm that much and being there also, they make sure that Init() is called (that the correct variables are NULL).

    A similarly superfluous method is AssignNewPointer(). In C++ tutorials, this is commonly shown as an example of a bad function:
    Code:
    int add(int a, int b) { return a + b; }
    AssignNewPointer() is just that: a single, simple statement wrapped in a function name.
    Not to mention, the main reason it took me so long to get into this code is that it is so all over the place. You seem to have a lot of redundancy in your implementation, leading to hard to understand code.
    I don't remember if the function did something else before, but as you say, I'll remove it.

    8) Various minor issues.
    Your Realloc() and ReallocElements() methods call realloc() on memory allocated with new. That's a big no-no. Worse, you completely misunderstand the way realloc() works and thus, in addition to invoking undefined behaviour due to memcpying non-PODs, you also leak the entire memory if realloc() has to relocate, and you're left with a dangling pointer, too.
    That's not good, huh? I will have to find another memory re-allocation function if so, but you can always throw me suggestions.

    Operator + is indeed dangerous and best removed.
    Done.

    Allocating a new, special object just to assign null to a pointer? WTF??? Not to mention that people might be tempted to compare pointers against PPNULL (It makes sense. After p = PPNULL; it should be expected that p == PPNULL. Not only is the comparison false, it also leaks memory.)
    Well, I think initially because of compiler's habit of casting any type to int. I can't seem to fix it either. Making an int constructor explicit only cuses compile errors when I try to return NULL from a function that return a CMemoryManager object.

    Why is the reference count not directly embedded in the PointerInfo struct? Why allocate it separately? Doing that just exposes you to problems.
    Ah, of course, my mistake. It was a pointer before, because it wasn't attached in a struct at all. I got the idea to actually keep more information about each pointer, so I put them into a struct.

    What's this TempPointerInfo? Why the name? An hour has passed now since I started looking at your post, and still I don't really have an idea.
    It's used for the map and caching. It puts the actual PointerInfo pointer (for the current memory) in there, along with the pointer (memory pointer, p) it's attached to and if I've looked up the current p in the map yet.
    Basically, the flow should be:
    operator = called with a new address.
    ArrayIsMemory() is called and check if the the pointer passed has been looked up in the map yet (like if you assign the class the same pointer twice, it won't look up the pointer info in the map again) and if it hasn't, it does. Then it fills the struct that it has looked up p and the PointerInfo* for p.
    When LookupSetRefCount() is called, it does a similar check and will find that the information has already been looked up (because m_PTempInfo.bLookedUp == true && m_PInfo.p == p) so it doesn't need to look it up in the map again but goes ahead and replaces the current m_PointerInfo with the one found in the map.

    I've edited the first post with the updated code.
    Also I seem to have problems declaring operator = and operator == explicit :/ I'm not sure that means the compiler can make stupid code again and call the operator with a non-int.

    Quote Originally Posted by brewbuck View Post
    I'm not saying it looks poor. I'm just not sure what you plan on doing with it.
    Of course, to manage my obsessity with pointers and objects on the heap. I'm already using the class, of course.

    Quote Originally Posted by cpjust View Post
    Well I don't really have time to read the whole thing (plus my brain is melting from looking at really bad C code all day), but if you're trying to create something like an auto_ptr class, I wrote my own a few years ago. You can check how it works and maybe get some ideas (I basically stole, err, I mean borrowed most of it from the STL auto_ptr class):
    http://tech.groups.yahoo.com/group/boost/files/AutoPtr/
    Oh crap. Sign in to Yahoo >_< No thanks. No offense to you, just to Yahoo.
    Last edited by Elysia; 10-31-2007 at 10:40 AM.

  8. #8
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    How can you not like Yahoo?
    Well here's the file anyways...
    Attachment 7606

    And here are the files for the example/test program that uses the AutoPtr:
    Attachment 7607
    Attachment 7608

  9. #9
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    I have no interest in google, yahoo, msn, etc, etc, and I find it annoying to sign up to view/download something which seems so popular today.

  10. #10
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Quote Originally Posted by Elysia View Post
    This is my thought too. It should always be const unless I have to do away with const.
    No, you misunderstand me. It should never be const. The class deals with a pointer to T. Not with a pointer to const T. If the user wants a pointer-to-const, he can do CMemoryManager<const CFoo>.

    Of course, bug. Now it should be fixed. I don't know if I should require atomic and use InterlockedIncrement/Decrement. The stupid functions require a signed LONG. Locks should be enough, or should it?
    Locks are enough, provided the compiler recognizes them as memory barriers. (I'm pretty sure it does.) Of course, making the refcount a LONG wouldn't hurt either, seeing how you're unlikely to ever get even close to two billion references to an object.


    I don't find the design bloated at all. It provides common access to functions you might use with pointers. What exactly do you find bad?
    It tries to make decisions that should be compile-time issues runtime issues. That makes it more fragile and more overloaded. You should have different smart pointer classes for different purposes. You should have one smart pointer for array allocations (better yet, you should use a standard container for arrays) and one for single object allocations. (Boost has shared_ptr and shared_array.) You should have one smart pointer that is thread-safe and one that is not. This way, at least you can easily see from just looking at the declaration whether you're allowed to pass this pointer to another thread. You don't even have to look at the constructor call. (Boost doesn't have a thread-unsafe smart pointer. It's considered superfluous. Boost's shared_ptr uses lock-free reference counting and is quite fast.) The whole performance-sensitive thing is nonsense - in performance-critical situations, you don't use a reference-counted smart pointer at all. You have tight inner loops that use references to objects. And beside the absurd pointer recapture facility you have, the only thing your performance switch does is disable some checks that are only done in debug mode anyway. Get rid of the machinery, use the smart pointer only where you really want to keep a hold on it, and you'll find that the whole issue goes away.

    And the class should NEVER EVER point to a local variable.
    Of course it shouldn't. The whole point (pun not intended) of a smart pointer is that it manages memory.

    Meaning?
    Meaning you lost track of which function is responsible for deleting m_pInfo. That really shouldn't happen. That wouldn't happen if the implementation wasn't so complicated and all over the place.

    That's not good, huh? I will have to find another memory re-allocation function if so, but you can always throw me suggestions.
    There isn't one. There's allocating a new block and copy-constructing the objects over. That's what std::vector does.

    It's used for the map and caching. It puts the actual PointerInfo pointer (for the current memory) in there, along with the pointer (memory pointer, p) it's attached to and if I've looked up the current p in the map yet.
    And you can't do that in local variables and function parameters, why? Note that you never need this data after initialization, yet your class carries (taking likely alignment issues into account) 3*sizeof(void*) bytes (that's 12 on 32-bit systems, 24 on 64-bit systems) around all its lifetime, even duly copies the zeros over on copy construction. (Or does it? Or does it leave those values uninitialized?)

    Basically, the flow should be:
    operator = called with a new address.
    ArrayIsMemory() is called and checks if the the pointer passed has been looked up in the map yet and if it hasn't, it does.
    Wonderfully convoluted. It's completely non-obvious that a function called ArrayIsMemory() does the lookup. You should simply do the lookup and have it set class members, and only if it fails do anything else.

    When LookupSetRefCount() is called, it does a similar check and will find that the information has already been looked up
    Alarm bells should be ringing in your head when you write stuff like that. There obviously is a lack of clearly defined responsibilities here.

    Also I seem to have problems declaring operator = and operator == explicit
    You can't. Only constructors can be explicit.

    I'm not sure that means the compiler can make stupid code again and call the operator with a non-int.
    It can't use the functions for implicit conversions. It can still implicitly convert the arguments from something else, do something you didn't quite expect, compile code you didn't expect to compile.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  11. #11
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by CornedBee View Post
    No, you misunderstand me. It should never be const. The class deals with a pointer to T. Not with a pointer to const T. If the user wants a pointer-to-const, he can do CMemoryManager<const CFoo>.
    Then it's impossible to send the class through a const reference since it can only call const functions.

    Locks are enough, provided the compiler recognizes them as memory barriers. (I'm pretty sure it does.) Of course, making the refcount a LONG wouldn't hurt either, seeing how you're unlikely to ever get even close to two billion references to an object.
    Shrug. You never know.

    It tries to make decisions that should be compile-time issues runtime issues. That makes it more fragile and more overloaded. You should have different smart pointer classes for different purposes.
    I'm not going to have 1000 classes that does the same thing. Splitting the class would be difficult or very tiresome as I'd have to rewrite a lot of code (since calling Release calls InternalDelete which calls InternalDetach, which Locks, deletes memory and removes pointer from map).
    Ah, unless I make virtual functions and override those with non-locking and non-pointer looking-uping.
    Maybe later.

    The whole performance-sensitive thing is nonsense - in performance-critical situations, you don't use a reference-counted smart pointer at all. You have tight inner loops that use references to objects. And beside the absurd pointer recapture facility you have, the only thing your performance switch does is disable some checks that are only done in debug mode anyway. Get rid of the machinery, use the smart pointer only where you really want to keep a hold on it, and you'll find that the whole issue goes away.
    Not nonsense in my view. Try assigning a lot of pointers to the class a lot of the time and you'll see a performance hit. But if you still want to use the class with heavy memory usage, you can easily use the performance critical type.

    Meaning you lost track of which function is responsible for deleting m_pInfo. That really shouldn't happen. That wouldn't happen if the implementation wasn't so complicated and all over the place.
    No, that was a bug. Initially, I just wanted to delete m_pInfo, but later realized that I had to delete m_pInfo->pdwRefCount, so I made a new function to handle that and called it but forgot to remove delete m_pInfo.

    There isn't one. There's allocating a new block and copy-constructing the objects over. That's what std::vector does.
    That's really stupid. I don't see why I can't use realloc, though. Even though you disagree, it can work, as I have used it in the past and is has worked fine. I don't know the implications of doing it, however.

    And you can't do that in local variables and function parameters, why? Note that you never need this data after initialization, yet your class carries (taking likely alignment issues into account) 3*sizeof(void*) bytes (that's 12 on 32-bit systems, 24 on 64-bit systems) around all its lifetime, even duly copies the zeros over on copy construction. (Or does it? Or does it leave those values uninitialized?)
    It simply doesn't work that way. Instead of looking up twice (I agree that could be done with arguments), but caching wouldn't work that way. Though I could remove the caching. I don't know if this would be a good idea, but I don't think it will have much of a performance impact.

    Wonderfully convoluted. It's completely non-obvious that a function called ArrayIsMemory() does the lookup. You should simply do the lookup and have it set class members, and only if it fails do anything else.
    Well, maybe I should do a separate Lookup() function that looks it up (if it hasn't been already) and when it returns (unless an exception is thrown), the function that called Lookup can check the m_PInfoTemp var for info it needs.

    You can't. Only constructors can be explicit.
    It can't use the functions for implicit conversions. It can still implicitly convert the arguments from something else, do something you didn't quite expect, compile code you didn't expect to compile.
    Now that is a shame, because I don't want the compiler to do all sorts of weird conversions before calling those operators.
    And now I have to post the newest code since I can't edit the first post:
    What's mostly new is Resize with realloc works as it should now (or so I think) and exception casting if something goes wrong. It also implememts a stack system, so it's possible to see the functions, in order, after the exception was thrown. Dunno how much use it has, but it seemed cool to me.

  12. #12
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Quote Originally Posted by Elysia View Post
    Now that is a shame, because I don't want the compiler to do all sorts of weird conversions before calling those operators.
    That's just a fact of life when dealing with integer-like types (including bool). You can wrap fundamental types in simple wrappers which will prevent automatic conversions, but you have to make their constructors explicit or you get all the same problems again. And explicit constructors means you have to do something silly like:

    Code:
    myFuncWhichTakesBool(MyBool(true));
    Instead of just:

    Code:
    myFuncWhichTakesBool(true);

  13. #13
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by brewbuck View Post
    That's just a fact of life when dealing with integer-like types (including bool). You can wrap fundamental types in simple wrappers which will prevent automatic conversions, but you have to make their constructors explicit or you get all the same problems again. And explicit constructors means you have to do something silly like:

    Code:
    myFuncWhichTakesBool(MyBool(true));
    Instead of just:

    Code:
    myFuncWhichTakesBool(true);
    Now that's something that should make it into the standard: no weird automatic conversions. If a function takes a bool, then it takes a bool, end of story. Not some int or any other kind of data type that the compiler can think of making up.

  14. #14
    The larch
    Join Date
    May 2006
    Posts
    3,573
    That's really stupid. I don't see why I can't use realloc, though. Even though you disagree, it can work, as I have used it in the past and is has worked fine. I don't know the implications of doing it, however.
    Can I use realloc() on pointers allocated via new?

    For one thing consider a class that needs to notify some other classes if its address changes. And I'm not sure if you are even guaranteed to be able to delete memory if the pointer has gone through realloc.

    Anyway, how comes a smart pointer is moving objects around? Shouldn't it manage pointers? (May-be leave the first task to dedicated classes such as std::vector?)

    Now that's something that should make it into the standard: no weird automatic conversions. If a function takes a bool, then it takes a bool, end of story. Not some int or any other kind of data type that the compiler can think of making up.
    I don't see what's the problem with that. You are writing classes for programmers (including yourself) and not idiots. It's the users problem if they don't bother to look up references before starting to pass random and inappropriate values...

    The users will always find a way to misuse your class. For example what happens if I delete the pointer that I have just given to a smart pointer?

    By the way, MSVC has a performance warning: forcing integer into bool (or something like that). I suppose to get rid of it, I'd have to be more explicit (although I'm not sure how that would improve performance):
    Code:
    int n = 10;
    functionThatTakesBool(n); //bad
    functionThatTakesBool(n != 0); //good?
    Last edited by anon; 10-31-2007 at 03:55 PM.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  15. #15
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by anon View Post
    Anyway, how comes a smart pointer is moving objects around? Shouldn't it manage pointers? (May-be leave the first task to dedicated classes such as std::vector?)
    I don't get what you mean?

    I don't see what's the problem with that. You are writing classes for programmers (including yourself) and not idiots. It's the users problem if they don't bother to look up references before starting to pass random and inappropriate values...

    The users will always find a way to misuse your class. For example what happens if I delete the pointer that I have just given to a smart pointer?
    It helps catch bugs at design time and not create weird bugs in your code when the compiler calls the entirely wrong function. I was frustrated before when I patched
    Code:
    myclass = 50;
    ...to the bool constructor that took a param that said if you wanted to create a new object when creating the class or not!
    It should have failed by then (code has since been revised).

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Message class ** Need help befor 12am tonight**
    By TransformedBG in forum C++ Programming
    Replies: 1
    Last Post: 11-29-2006, 11:03 PM
  2. Direct3D problem
    By cboard_member in forum Game Programming
    Replies: 10
    Last Post: 04-09-2006, 03:36 AM
  3. My Window Class
    By Epo in forum Game Programming
    Replies: 2
    Last Post: 07-10-2005, 02:33 PM
  4. base class pointer problems
    By ... in forum C++ Programming
    Replies: 3
    Last Post: 11-16-2003, 11:27 PM
  5. Warnings, warnings, warnings?
    By spentdome in forum C Programming
    Replies: 25
    Last Post: 05-27-2002, 06:49 PM