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("%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

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.

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?

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/

Quote:

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.

It may look poor to you because I'm pretty much self-learned :p
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!

Quote:

Originally Posted by Elysia

It may look poor to you because I'm pretty much self-learned :p

I'm not saying it looks poor. I'm just not sure what you plan on doing with it.

Quote:

Originally Posted by CornedBee

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.

Quote:

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.

Quote:

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).

Quote:

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?

Quote:

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.

Quote:

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.

Quote:

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

Meaning?

Quote:

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).

Quote:

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.

Quote:

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.

Quote:

Operator + is indeed dangerous and best removed.

Done.

Quote:

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.

Quote:

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.

Quote:

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

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

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.

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

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.

Quote:

Originally Posted by Elysia

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>.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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?)

Quote:

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.

Quote:

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.

Quote:

Also I seem to have problems declaring operator = and operator == explicit

You can't. Only constructors can be explicit.

Quote:

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.

Quote:

Originally Posted by CornedBee

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

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.

Quote:

Originally Posted by Elysia

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);

Quote:

Originally Posted by brewbuck

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.

Quote:

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?)

Quote:

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?

Quote:

Originally Posted by anon

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?

Quote:

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).

Quote:

Originally Posted by Elysia

Then it's impossible to send the class through a const reference since it can only call const functions.

You're confusing const pointers with pointers-to-const. It's the same for smart pointers as for raw pointers: there's a difference between "CFoo const *" and "CFoo * const", and there's a difference between "smart_pointer<CFoo const>" and "const smart_pointer<CFoo>".

Quote:

Shrug. You never know.

Look at it this way: on a 32-bit system, your class is currently probably 7*sizeof(void*) = 28 bytes large. To have the refcount rise to 2^31 (the overflow point), you'd need 2^31 smart pointers occupying 2^31 * 28 bytes, that's 2^32 * 7 bytes, and that's more than a 32-bit CPU is capable of addressing. So yes, you do know.
In the 64-bit case, it might work in theory. Your class is 56 bytes large (that's huge, by the way - another reason to get rid of all that runtime stuff; a refcounting smart pointer should be 16 bytes large), which is 2^33 * 7 bytes. This does fit into the 2^40 bytes an Athlon64 can physically address.
However, the stack is still limited way below that even in 64-bit systems. And these smart pointers are intended for stack usage - you're actually trying to block heap allocation.

Quote:

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).

Bzuh? If you followed my advice, you'd be using Boost's smart pointers and not writing a single line of code.

Quote:

Ah, unless I make virtual functions and override those with non-locking and non-pointer looking-uping.

No. Bad. You can use base classes for implementation sharing, but no virtuals. Virtuals are for runtime decisions. You should get rid of those.

Quote:

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.

1) You shouldn't be assigning a lot of pointers to the class a lot of the time. If you do, you've got a logical problem in your code. You're busy juggling pointers when you should be doing work.
2) What the hell are you doing that takes so long? Oh, I know. You're looking stuff up in your various global maps.

Quote:

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.

My point exactly.

Quote:

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.

It's a necessary consequence of encapsulation. You don't know the internals of a class. You can't know if it's safe to copy it to someplace else. The class might be holding a pointer to its own members. (Objects using the small buffer optimization often do that.) You have to call the class's copy constructor to create an instance somewhere else. And because realloc is a C function, it doesn't know that. It doesn't even know that such things as copy constructors exist.
For that matter, you don't know if operator new is even implemented in terms of malloc. An implementation is free not to do so. A programmer is free to override the implementation's default implementation. Calling realloc assumes that malloc allocated the block in the first place.
It's pretty much a time bomb waiting to blow up in your face.

Quote:

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.

What caching?

Quote:

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.

No chance. The standard won't break backwards compatibility, and there's actually code out there that relies on these conversions.

Quote:

Originally Posted by CornedBee

You're confusing const pointers with pointers-to-const. It's the same for smart pointers as for raw pointers: there's a difference between "CFoo const *" and "CFoo * const", and there's a difference between "smart_pointer<CFoo const>" and "const smart_pointer<CFoo>".

Code:

void foo(const pp<int>& p) { int n = *p; // No workie because operator * and operator T* isn't const. } int main() { ppnew<int> p(50); foo(p); return 0; }

Quote:

Look at it this way: on a 32-bit system, your class is currently probably 7*sizeof(void*) = 28 bytes large. To have the refcount rise to 2^31 (the overflow point), you'd need 2^31 smart pointers occupying 2^31 * 28 bytes, that's 2^32 * 7 bytes, and that's more than a 32-bit CPU is capable of addressing. So yes, you do know.
In the 64-bit case, it might work in theory. Your class is 56 bytes large (that's huge, by the way - another reason to get rid of all that runtime stuff; a refcounting smart pointer should be 16 bytes large), which is 2^33 * 7 bytes. This does fit into the 2^40 bytes an Athlon64 can physically address.
However, the stack is still limited way below that even in 64-bit systems. And these smart pointers are intended for stack usage - you're actually trying to block heap allocation.

Ah well, but as the saying goes: What isn't broken, don't fix. It can remain unsigned and the critical section will take care of the locking, so it shouldn't be a problem.

Quote:

Bzuh? If you followed my advice, you'd be using Boost's smart pointers and not writing a single line of code.

No chance :)

Quote:

No. Bad. You can use base classes for implementation sharing, but no virtuals. Virtuals are for runtime decisions. You should get rid of those.

No. Good. Polymorphism is good. If I don't use Virtuals, I'll have to tear the class apart and re-write already existing code when creating new ones. THAT is bad.

Quote:

1) You shouldn't be assigning a lot of pointers to the class a lot of the time. If you do, you've got a logical problem in your code. You're busy juggling pointers when you should be doing work.
2) What the hell are you doing that takes so long? Oh, I know. You're looking stuff up in your various global maps.

And that's why I added time critical - it doesn't mess with the maps!

Quote:

My point exactly.

Point was that I forgot to remove a line - I never forgot what function did what! And it's better than copying around all the code all the time.

Quote:

It's a necessary consequence of encapsulation. You don't know the internals of a class. You can't know if it's safe to copy it to someplace else. The class might be holding a pointer to its own members. (Objects using the small buffer optimization often do that.) You have to call the class's copy constructor to create an instance somewhere else. And because realloc is a C function, it doesn't know that. It doesn't even know that such things as copy constructors exist.
For that matter, you don't know if operator new is even implemented in terms of malloc. An implementation is free not to do so. A programmer is free to override the implementation's default implementation. Calling realloc assumes that malloc allocated the block in the first place.
It's pretty much a time bomb waiting to blow up in your face.

I just learned it doesn't call copy constructors... Yes, I know what that means. I never thought of it. Look like we're going to have to do a some good ol' memory copying to new memory, which is expensive and stupid when you can simply resize an existing memory block. Bah.
BTW, there's no C++ safe copying method? memcpy and CopyMemory are obviously bad.

Quote:

What caching?

Tut-tut. m_PInfoTemp caches the lookup for pointer p so it doesn't have to look up twice or more if it's going to lookup p again.

Quote:

No chance. The standard won't break backwards compatibility, and there's actually code out there that relies on these conversions.

Doesn't necessarily mean make it default behaviour. Add something along the lines of explicit or mutable or another keyword for it. Safe for everyone.

Quote:

Originally Posted by Elysia

Doesn't necessarily mean make it default behaviour. Add something along the lines of explicit or mutable or another keyword for it. Safe for everyone.

My suggestion of wrapping up types inside objects with explicit constructors would accomplish exactly what you are talking about.

It doesn't work like it should:

Code:

pp<CRect> foo() { return NULL; // error C2664: 'CMemoryManager<T>::CMemoryManager(const CMemoryManager<T> &)' : cannot convert parameter 1 from 'int' to 'const CMemoryManager<T> &' } int main() { foo(); return 0; } explicit CMemoryManager(int/* nNull*/);

Remove explicit and it works. But it also works to return a bool >_< Which I why I used PPNULL in the first place.

Quote:

Originally Posted by Elysia

I don't get what you mean?

I mean I'm quite perplexed by the resize methods (seeing how each allocates):

Code:

void Resize(size_t dwNewSize) throw(...) { //... const void* pTemp = realloc(p, dwNewSize); //... } void ResizeElements(DWORD dwNewSize) throw(...) { //... const void* pTemp = realloc(p, sizeof(*p) * dwNewSize); //... }

Even if realloc would work here, this seems like an accident waiting to happen...

But if you need to resize dynamic arrays, wouldn't it be wiser to use a std::vector in the first place?

Quote:

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).

This is a case that making the constructor explicit should catch.

However, in your code I see you using the explicit keyword with a constructor that takes two parameters. I can't see how that constructor could be called implicitly otherwise.

Quote:

Originally Posted by anon

I mean I'm quite perplexed by the resize methods (seeing how each allocates):

Code:

void Resize(size_t dwNewSize) throw(...) { //... const void* pTemp = realloc(p, dwNewSize); //... } void ResizeElements(DWORD dwNewSize) throw(...) { //... const void* pTemp = realloc(p, sizeof(*p) * dwNewSize); //... }

Even if realloc would work here, this seems like an accident waiting to happen...

But if you need to resize dynamic arrays, wouldn't it be wiser to use a std::vector in the first place?

Yes, they should be removed (or maybe renamed). Or maybe I should rename them to ResizeC? Err, but C doesn't support classes. Hmmm. Maybe I should deprecate arrays and demand the use of vectors instead... That might be a good idea when I think about it.

Quote:

This is a case that making the constructor explicit should catch.

Doesn't work. See above ^

Quote:

However, in your code I see you using the explicit keyword with a constructor that takes two parameters. I can't see how that constructor could be called implicitly otherwise.

Better safe than sorry.

Quote:

Look like we're going to have to do a some good ol' memory copying to new memory, which is expensive and stupid when you can simply resize an existing memory block. Bah.

You seem to think that realloc magically resizes an existing memory block. It can only do so because it overallocates in the first place. And if there simply isn't enough room to expand it will move everything to a new place.

Code:

#include <iostream> #include <cstdlib> int main() { int size = 2; int* p = (int*)malloc(sizeof (int) * size); std::cout << p << '\n'; for (int i = 0; i != 10; ++i) { size += 128; p = (int*)realloc(p, size* sizeof (int)); std::cout << p << '\n'; } free(p); }

Output:

Quote:

0x3d37e8
0x3d3850
0x3d3850
0x3d3850
0x3d3e60
0x3d3e60
0x3d3e60
0x3d3e60
0x3d3e60
0x3d4e70
0x3d4e70

As you can see the memory address is changing (everything gets moved at these places). Actually it is not different how std::vector resizes itself (by overallocating), except that std::vector works properly with user-defined objects.

Quote:

BTW, there's no C++ safe copying method? memcpy and CopyMemory are obviously bad.

Yes, these are bad. A good way (performance-wise) is to overallocate like std::vector does (which makes me ask, why a MemoryManager or SmartPointer should be managing dynamic arrays).

Quote:

Originally Posted by anon

...(which makes me ask, why a MemoryManager or SmartPointer should be managing dynamic arrays).

Because the app should not have access to the pointer and should in NO circuimstance delete or re-allocate memory in that memory because it's owned by the CLASS and NOT the function.

Quote:

Originally Posted by Elysia

Because the app should not have access to the pointer and should in NO circuimstance delete or re-allocate memory in that memory because it's owned by the CLASS and NOT the function.

But that's the case with normal usage of std::vector. Sure you can obtain the pointer (address of first element) and try to delete it but you'd need to be rather dumb to do that. I guess functions that need to re-allocate memory, should take a vector as an argument, not a raw pointer.

The only problem I can see right now are buffers. Arrays full of chars or BYTES. A vector won't suit for that purpose and I need to resize the darn thing, but I guess the task of a buffer should fall on a different class.

>> Arrays full of chars or BYTES. A vector won't suit for that purpose and I need to resize the darn thing, but I guess the task of a buffer should fall on a different class.

Why won't a vector be suitable for that?

Would it? I don't see vector as the sort of class that holds buffers.

I would imagine it would be great for that, but maybe our understandings of "buffer" are different.

Quote:

Originally Posted by Elysia

The only problem I can see right now are buffers. Arrays full of chars or BYTES. A vector won't suit for that purpose and I need to resize the darn thing, but I guess the task of a buffer should fall on a different class.

A vector<unsigned char> is perfect for that...

What's wrong with vector::reserve(), vector::resize() or just letting the vector grow on its own?

You really should read this book... It answers virtually all the questions that we're talking about.

Chapter 76. Use vector by default. Otherwise, choose an appropriate container.

Quote:

If you doubt this advice, ask yourself if you really have a compelling reason not to use the only standard container that guarantees all of the following properties. vector alone is:

Guaranteed to have the lowest space overhead of any container (zero bytes per object).
Guaranteed to have the fastest access speed to contained elements of any container.
Guaranteed to have inherent locality of reference, meaning that objects near each other in the container are guaranteed to be near each other in memory, which is not guaranteed by any other standard container.
Guaranteed to be layout-compatible with C, unlike any other standard container. (See Items 77 and Chapter 78)
Guaranteed to have the most flexible iterators (random access iterators) of any container.
Almost certain to have the fastest iterators (pointers, or classes with comparable performance that often compile away to the same speed as pointers when not in debug mode), faster than those of all other containers.

Chapter 77. Use vector and string instead of arrays

Chapter 78. Use vector (and string::c_str) to exchange data with non-C++ APIs

Code:

std::vector<BYTE> v; v.push_back(0); v.reserve(10000); BYTE& p = v[0]; ZeroMemory(&p, 10000);

Good or bad?

Quote:

Originally Posted by Elysia

I haven't looked into the internals of the vector class, but does it actually put these elements next to each other, like a native array?

Yes, see my last post.

I just missed it when replying.
Anyway, I rewrote some code to use a vector instead of the memory manager. However, I shouldn't blame myself if I get lots of memory errors / faults, since the code was written with C-style buffers in mind, using pointers and keeping track of when needing to resize and so on.
Deprecated array support for class. Will remove if there are no prolems. Now I'm thinking of going to bed because I'm tired.
Fun discussing with you all, though.

Assuming ZeroMemory() is zeroing elements in an array-based fashion (p[0] = 0, p[1] = 0, ...), you should use resize instead of reserve, since reserve only sets v's capacity, not its size (or just use the constructor

Code:

std::vector<BYTE> v(10000);

which creates v with size 10000, eliminating the need for the reserve). Personally I would have written this as

Code:

std::vector<BYTE> v; v.reserve(10000); for (int i=0; i<10000; ++i) v.push_back(0);

since using push_back() exclusively takes care of the size automatically (you don't ever actually need reserve(), it's just an optimization and never affects correctness).

Edit: I'm not sure whether the 1-argument constructor mentioned above is guaranteed to zero-initialize the elements the way you want - if it is, you could just write

Code:

std::vector<BYTE> v(10000);

and be done with it. Anyone know?

Quote:

Originally Posted by Elysia

Code:

void foo(const pp<int>& p) { int n = *p; // No workie because operator * and operator T* isn't const. }

So make them. My point was that you shouldn't be dealing with const T* in your class.

Quote:

Ah well, but as the saying goes: What isn't broken, don't fix. It can remain unsigned and the critical section will take care of the locking, so it shouldn't be a problem.

True. I was just pointing out that the reduced maximum of LONG vs DWORD isn't realistically any obstacle.

Quote:

No. Good. Polymorphism is good. If I don't use Virtuals, I'll have to tear the class apart and re-write already existing code when creating new ones. THAT is bad.

Wrong. You don't. You can use base classes for implementation sharing. shared_ptr and shared_array do it.
Polymorphism is for when you actually have a reference to the base class. But an implementation-sharing base class should never be referenced. It should be an implementation detail.
Polymorphism is for runtime decisions.

Quote:

BTW, there's no C++ safe copying method? memcpy and CopyMemory are obviously bad.

std::copy() is as safe as anything. std::uninitialized_copy() if the target area doesn't have objects in it yet.

Quote:

Tut-tut. m_PInfoTemp caches the lookup for pointer p so it doesn't have to look up twice or more if it's going to lookup p again.

But the mere fact that p is set ought to be enough information.
Also, it again comes down to having a single place where lookup is done. And by that I don't mean just a single function, because you can still call the function multiple times. I mean that, in the flow chart of pointer construction/assignment, the lookup is only done once.

Quote:

It doesn't work like it should:

Code:

pp<CRect> foo() { return NULL; // error C2664: 'CMemoryManager<T>::CMemoryManager(const CMemoryManager<T> &)' : cannot convert parameter 1 from 'int' to 'const CMemoryManager<T> &' }

Remove explicit and it works. But it also works to return a bool >_< Which I why I used PPNULL in the first place.

The standard idiom for getting a null smart pointer is to default construct it, i.e.
return CMemoryManager<T>();

Quote:

Better safe than sorry.

explicit only has an effect on constructors that can be called with one argument. The keyword is completely ignored otherwise. So you're not in any way safe.

Quote:

Originally Posted by robatino

Assuming ZeroMemory() is zeroing elements in an array-based fashion (p[0] = 0, p[1] = 0, ...), you should use resize instead of reserve, since reserve only sets v's capacity, not its size (or just use the constructor

Code:

std::vector<BYTE> v(10000);

which creates v with size 10000, eliminating the need for the reserve). Personally I would have written this as

Code:

std::vector<BYTE> v; v.reserve(10000); for (int i=0; i<10000; ++i) v.push_back(0);

since using push_back() exclusively takes care of the size automatically (you don't ever actually need reserve(), it's just an optimization and never affects correctness).

Edit: I'm not sure whether the 1-argument constructor mentioned above is guaranteed to zero-initialize the elements the way you want - if it is, you could just write

Code:

std::vector<BYTE> v(10000);

and be done with it. Anyone know?

I just wanted to copy something into the array and make sure there were no errors when deleting that array later (by std::vector of course).

Quote:

Originally Posted by CornedBee

So make them. My point was that you shouldn't be dealing with const T* in your class.

But I do not see why not. If the functions do not change the T*, why not const? If they try to assign it somewhere, obviously it should not be const. So far as I'm aware, const is to catch design time bugs mostly.

Quote:

Wrong. You don't. You can use base classes for implementation sharing. shared_ptr and shared_array do it.
Polymorphism is for when you actually have a reference to the base class. But an implementation-sharing base class should never be referenced. It should be an implementation detail.
Polymorphism is for runtime decisions.

Then explain exactly how I can implememt a thread-safe class without re-writing the code. The thread-safety is done is protected functions done in the base class. Merely overriding them in the derived class won't get me anywhere, not unless I make them virtual.
Without that, I don't see how to do it without re-writing the entire darn class, which is not something I want to do.

Quote:

But the mere fact that p is set ought to be enough information.
Also, it again comes down to having a single place where lookup is done. And by that I don't mean just a single function, because you can still call the function multiple times. I mean that, in the flow chart of pointer construction/assignment, the lookup is only done once.

I don't feel that's a good idea. To make the flow chart dynamic is better, since it allows me to do what must be done if it hasn't been done already. Dynamic flowchart also decreases the chance for example, to access a NULL pointer (because the lookup function wasn't called).
It's also what I've built the class upon - dynamic flowchart. Determine what needs to be done and do it. So simple, yet so effective.

Quote:

The standard idiom for getting a null smart pointer is to default construct it, i.e.
return CMemoryManager<T>();

Sounds better to me to use PPNULL.

Quote:

explicit only has an effect on constructors that can be called with one argument. The keyword is completely ignored otherwise. So you're not in any way safe.

And not even a warning from the compiler. How selfish of it.

Quote:

Originally Posted by Elysia

But I do not see why not. If the functions do not change the T*, why not const?

Because it's not the pointer's responsibility to ensure that the pointee doesn't change. Or in other words, you don't need const T inside your pointer class because you have the design error the moment you dereference the pointer, not the moment you change it. The only exception is the pointer's own operator* overload.
It comes down to the pointer behaving like a raw pointer where it matters. A raw pointer doesn't care about whether its pointee is const or not. That's for the user of the pointer to worry about. The pointer just points and carries the constness along unchanged.
Your pointer changes the constness, and that's bad.

Quote:

Then explain exactly how I can implememt a thread-safe class without re-writing the code. The thread-safety is done is protected functions done in the base class. Merely overriding them in the derived class won't get me anywhere, not unless I make them virtual.
Without that, I don't see how to do it without re-writing the entire darn class, which is not something I want to do.

Gimme an hour (but it may be interrupted by lunch), and I'll show you.

Quote:

I don't feel that's a good idea. To make the flow chart dynamic is better, since it allows me to do what must be done if it hasn't been done already.

A static flowchart approach allows you to construct reliable flowcharts where these checks aren't necessary except as debug-mode asserts. Thus, your pointer is reliable without the runtime overhead of checking whether it's done its thing already.
Your approach is basically, write potentially buggy code, but instead of finding the bugs and solving them, you look for the effects of the bugs and solve those. You're curing the symptoms, not the illness.

Quote:

Dynamic flowchart also decreases the chance for example, to access a NULL pointer (because the lookup function wasn't called).

A static flowchart allows you to analyze at design time if a NULL pointer can be accessed, without any runtime overhead. Your dynamic approach makes this considerably harder.

Quote:

It's also what I've built the class upon - dynamic flowchart. Determine what needs to be done and do it. So simple, yet so effective.

No, it's neither simple nor effective. It's complex in implementation, and leads to code repetition and unnecessary performance overhead, while not increasing reliability.

Quote:

Sounds better to me to use PPNULL.

*shrug* Matter of habit, I suppose.
Note that the idiom is consistent, though:

Code:

typedef int* pint; pint p = pint(); assert(p == 0);

Quote:

And not even a warning from the compiler. How selfish of it.

Well, why would it warn you? Compilers only generate warnings for things that are potentially problematic.

Quote:

Originally Posted by CornedBee

Gimme an hour (but it may be interrupted by lunch), and I'll show you.

Sure. Looking forward to that.

Quote:

A static flowchart approach allows you to construct reliable flowcharts where these checks aren't necessary except as debug-mode asserts. Thus, your pointer is reliable without the runtime overhead of checking whether it's done its thing already.
Your approach is basically, write potentially buggy code, but instead of finding the bugs and solving them, you look for the effects of the bugs and solve those. You're curing the symptoms, not the illness.

A static flowchart allows you to analyze at design time if a NULL pointer can be accessed, without any runtime overhead. Your dynamic approach makes this considerably harder.

No, it's neither simple nor effective. It's complex in implementation, and leads to code repetition and unnecessary performance overhead, while not increasing reliability.

I cannot agree. From what I see, it makes the code more robust and safe at the expense of a little more overhead.

Here's a good one:
Know any way to restrict this polymorphism?

Code:

pp<int>* pMemoryManagers = new tcpp<int>[nRange];

tcpp inherits from pp, so the compiler allows it, but I don't like the idea of doing that.
There also seems to be a problem with

Code:

pMemoryManagers[i] = pPointers[i];

pMemoryManagers is tcpp and pPointers is int*. However, as tcpp derives from CMemoryManager (pp), it should have such an operator because pp does:

Code:

void operator = (T* pmm) throw(...)

Which is also public.
Or maybe it's true that operators aren't inherited.

Here's another thing: when running out of memory, it's not possible to create and throw an exception. I could create a single exception object as a class member, but this makes the class bigger, of course.
A global object might require locking, but this would fail again when running out of memory. So what's one to do, really? Throw a constant?

> I just wanted to copy something into the array and make sure there were no errors when deleting that array
> later (by std::vector of course).

With vectors, you have to make sure that the size (not just the capacity) is at least N+1 before you access v[N] (the (N+1)-th element). Unfortunately, since bounds checking isn't done by default, using reserve() to set the capacity only tends to prevent crashing, leading one to believe that one's program is correct (I made this mistake when first learning to use vectors). Remember, reserve() NEVER affects correctness. If a program is broken without it, then it's broken with it. If you're using GCC, try compiling with -D_GLIBCXX_DEBUG to do extra run-time checks including bounds checking, and watch the "fixed" problem reappear.

http://gcc.gnu.org/onlinedocs/libstdc++/debug.html

Using VC++ (2005).

Quote:

Originally Posted by Elysia

Using VC++ (2005).

I'm not familiar with that compiler, but you can use at() instead of [] to cause run-time bounds checking (for example, v.at(N) instead of v[N]). So this code will probably not complain, without the extra checks:

Code:

#include <vector> int main() { std::vector<int> v; // creates v with size 0 v.reserve(1); v[0] = 0; // this element doesn't exist, since the size is still 0 }

but if you replace v[0] with v.at(0), it will definitely complain.

Edit: Forgot std::. BTW, definitely try to find out if your compiler has something analogous to -D_GLIBCXX_DEBUG, if you don't want to use at() all the time.

Quote:

Originally Posted by Elysia

I cannot agree. From what I see, it makes the code more robust and safe at the expense of a little more overhead.

Complex code is never more robust than simple code in the long run.

Quote:

Know any way to restrict this polymorphism?

Code:

pp<int>* pMemoryManagers = new tcpp<int>[nRange];

Uh, oh, dangerous. You're right in that you want to restrict it, but unfortunately, it's not fully possible. It's simply a lesson that C++ programmers have to learn that array new and derivation don't mix. So don't do it, and educate any programmer you find doing it. Item 3 in More Effective C++, Item 89 in C++ Gotchas.
You can partially protect yourself by declaring a class-specific array new and not implementing it. The code will fail at link time.

Quote:

Here's another thing: when running out of memory, it's not possible to create and throw an exception.

Depends on the kind of memory you run out of. If you run out of stack memory, your program is killed, and that's it. If you run out of heap memory, new will throw a bad_alloc. So obviously it is possible to throw, at least to a point.
Basically, if you run out of memory, you've got bigger fish to fry than what kind of exception to throw. Let new throw its bad_alloc.

OK, here's my smart pointer suite. It is syntax-checked, but I haven't tested it in any way, and it may still fail to compile when actually instantiated. Also, no implementation of atomic_add is provided, as this is highly system-specific. (InterlockedExchangeAndAdd() should be a good base for implementing it.)

This provides four smart pointer classes. They are the four combinations of two basic decisions:
1) Manage an array or a single object?
2) Be thread-safe or not?

There are no hooks to implement your map lookup. I remain firmly opposed to it and thus didn't accommodate it in my design.

However, there are hooks to implement weak pointers. Weak pointers maintain a link to the object, but do not increase the reference count. If the last strong pointer goes away, the object is deleted, even if weak pointers to it remain. The weak pointers know of the object's destruction.
In Boost's implementation, you cannot dereference weak pointers, but you can create strong pointers from them. Trying to create a strong pointer from an invalidated weak pointer throws an exception. The reason for this design is that it is unsafe to access the pointee if you don't have control over a strong pointer to it. If the weak pointer allowed direct access, the object could be deleted from under your feet.

The most complicated thing about it is, in my opinion, the CRTP. However, it's a pattern, which means that once you understand it, it provides no complexity overhead. In addition, it provides zero space or runtime overhead in the program itself compared to directly implementing the functionality. Everything is fully inlineable.

Edit: deleted attachment, has a bug. Added new version a few posts down.

It's a shame because I wanted to throw my own specific exception class, but when running out of memory it just doesn't seem like it works.
Newest header file attached.
Changes:
- Always thread safe.
- One time critical and one non-time critical class. Uses virtual functions to determine if to call the time critical functions or not.
- Not handling memory errors because if you run out of memory, the whole will likely crash.
- Fixed some overloading.
- Removed array support.
- Removed CMemoryManagerDefault.
- Reinstated PPNULL. Not possible to check against NULL, to construct a new NULL anymore (all checks should be done against PPNULL).

It probably does, but you're probably following MFC convention and allocating the exception from the heap, right?

Yes, should I try to throw it at the stack?
Problem is the string function will try to allocate from the heap and fail.

Then don't add a string, or provide just a small, static buffer. But really, is it that important?

Tried providing small, static buffers, but it just doesn't want to work right when other function try to allocate like MFC's idiotic CMemoryException class (which can't return an error through a function with a CString reference).
But ah... it doesn't really matter that much with memory shortage. Likely the whole program will crash anyway.

Oh, yeah, forgot another assumption my smart pointers make: that when given a raw pointer, they have exclusive access to it. If allocation of the share count fails, they will delete the pointer.

Eh, reminds me that I have a bug in there. Hold on a second.

Ah, there you go.

By the way:

Quote:

Uses virtual functions to determine if to call the time critical functions or not.

Do you realise that virtual functions carry a speed overhead?

Quote:

Originally Posted by CornedBee

By the way:

Do you realise that virtual functions carry a speed overhead?

'Course, but not much. Unless you have another way to do it without rewriting the class with the current implemtation, then...

BTW, instead of making different smart pointers for single vs arrays... My AutoPtr deletes memory in any way you want by passing in a custom "deletor" functor. You can write a deletor to do delete p; or delete [] p; or free( p ); or anything else you can think of.
Actually you could probably use it as a general purpose RAII object, maybe with some small changes...

Well, as you all have mentioned, vector is enough for arrays AND for buffers so support for arrays is not necessary.

The smart pointers in std::tr1 are probably enough for any other smart pointer needs your class is trying to achieve as well. I thought that wasn't the point.

I found another dangerous behaviour today:

Code:

pp< vector<CString> > strFiles = Stuff::FindFiles(Stuff::GetWindowText(m_Directory), m_Recursive.GetCheck() == 1, _T("*.*"), NULL, NULL, strFiles, false, NULL, NULL);

As far as I'm concerned, it shouldn't work at all since we're passing strFiles when we haven't declared it. What's worse is that the compiler will create strFiles and pass it to the function without calling the objects constructor, thus resulting in a crash.

Code:

int main() { int i = i + 0; std::string str = str + "World!"; }

Sometimes you can do bad things in C++. If there's nothing stopping you from doing it with int or string, then I wouldn't worry about it for your class.

Apparently this compiles without a warning:

Code:

#include <iostream> class A { public: A(): value(10) {} void print()const {std::cout << value << '\n';} private: int value; }; A foo(const A& a) { a.print(); return A(); } int main() { A a = foo(a); std::cin.get(); }

My output is 16384 (should be 10 if the constructor was called before print).

I guess this might be a problem of the language that it allows it. But then you should just avoid writing code that cannot possibly make sense (using an instance to construct the same instance) and stop worrying about particularly broken use cases when designing classes.

return A();

isn't that the equivalent of returning a local variable?

It's ok, because the function returns by value.

oh ok.

Quote:

Originally Posted by Daved

Code:

int main() { int i = i + 0; std::string str = str + "World!"; }

Sometimes you can do bad things in C++. If there's nothing stopping you from doing it with int or string, then I wouldn't worry about it for your class.

Does anyone know if that is a compiler bug or whether somebody missed that case when defining the C++ language?

It's not just VC++. Comeau online gives the following output:

Code:

Thank you for testing your code with Comeau C/C++! Tell others about http://www.comeaucomputing.com/tryitout ! Your Comeau C/C++ test results are as follows: Comeau C/C++ 4.3.9 (Mar 27 2007 17:24:47) for ONLINE_EVALUATION_BETA1 Copyright 1988-2007 Comeau Computing. All rights reserved. MODE:strict errors C++ C++0x_extensions "ComeauTest.c", line 5: warning: variable "i" is used before its value is set int i = i + 0; ^ In strict mode, with -tused, Compile succeeded (but remember, the Comeau online compiler does not link). Compiled with C++0x extensions enabled.

I don't feel like checking the standard at the moment. I'd imagine you'd have to combine several parts to come to a conclusion.

Having the smart pointer code embedded in both my DLL and my main app, it seems that TWO objects are created and the static members, the map and lock, are not the same across the DLL boundaries.
I'm guess I'm going to have to imbed it into the DLL and export the class to the app.

EDIT:
As usual, when I try to export the class, I get linking errors instead:

Code:

template<typename T> class AFX_EXT_CLASS CMemoryManager; 2>Do X For Every FileDlg.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall CMemoryManager<class std::vector<class ATL::CStringT<wchar_t,class StrTraitMFC_DLL<wchar_t,class ATL::ChTraitsCRT<wchar_t> > >,class std::allocator<class ATL::CStringT<wchar_t,class StrTraitMFC_DLL<wchar_t,class ATL::ChTraitsCRT<wchar_t> > > > > >::~CMemoryManager<class std::vector<class ATL::CStringT<wchar_t,class StrTraitMFC_DLL<wchar_t,class ATL::ChTraitsCRT<wchar_t> > >,class std::allocator<class ATL::CStringT<wchar_t,class StrTraitMFC_DLL<wchar_t,class ATL::ChTraitsCRT<wchar_t> > > > > >(void)" referenced in function "public: void __thiscall CDoXForEveryFileDlg::OnBnClickedBrowseapp(void)"

I was also unable to map these into definitions in the .cpp file:

Code:

template<typename NewT> operator CMemoryManager<NewT>& () const; template<typename T2> static CMemoryManager<T> MemoryManagerNew(T2* pNew) throw(...); CMemoryManagerTimeCritical& operator = (T* p) throw(...) { CMemoryManager<T>::operator = (p); return *this; } CMemoryManagerTimeCritical& operator = (CMemoryManagerTimeCritical<T>& r) throw(...); //template<typename T, typename T2> CMemoryManager<T> CMemoryManager<T>::MemoryManagerNew(T2* pNew) throw(...) //{ // MM_RETHROW_ERRORS_START; // CMemoryManager<T> mm; // T* pLocal = pNew; // mm.Attach(pLocal); // return mm; // MM_RETHROW_ERRORS_END("CMemoryManager::MemoryManagerNew(T* pNew)"); //} //template<typename T, typename NewT> CMemoryManager<T>::operator CMemoryManager<NewT>& () const //{ // 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; //} //MMC_TWO(CMemoryManagerTimeCritical<T>&) //template<typename T, typename T2> CMemoryManagerTimeCritical<T>& CMemoryManagerTimeCritical<T>::operator = (T2* p) throw(...) //{ // CMemoryManager<T>::operator = (p); // return *this; //} //template<typename T, typename T2> CMemoryManagerTimeCritical<T>& CMemoryManagerTimeCritical<T>::operator = (CMemoryManagerTimeCritical<T2>& r) throw(...) //{ // CMemoryManager<T>::operator = (r); // return *this; //} #define MMTC_TWO(return_type) template<typename T, typename T2> return_type CMemoryManagerTimeCritical<T>

Any ideas or suggestions?

You can't put a template in a DLL.

Edit: Clarifying: you can't export a template from a DLL. Except by using the compiler's extern template extension, for a given set of instantiations. But not the generic code.

Yes, I figured, so I need another solution to this.
So here's another problem:

Code:

CMemoryManager(const CMemoryManager<T>& rmm)

Can also take a class of type CMemoryManagerTimeCritical or any other class derived from it, which it should not, because they're not compatible.
I was able to block such an attempt by making a prive constructor that takes the specified types.

Creating a static variable in a different thread than the main will cause the object to be destructed on the main thread rather than the thread that created it. In Release mode, I also get an access violation due to freeing an invalid address. This may be a problem. It may be that static vars are dangerous?