Base container class question

I made this container, and it seems to work, but I may be missing something.

I was wondering if you all could critique it, and let me know what you think.

All input is greatly appreciated.

Code:

#pragma once
#include "VariaMacros.h"
#include "cSingletonBase.h"

#include <map>
#include <list>
#include <string>

template <typename T>
class cMapBase : public cSingletonBase<cMapBase<T> >
{
	EnableSingleton(cMapBase);
	DisableClassCopying(cMapBase);
private:
	cMapBase():m_CurIndex(0){};
	virtual ~cMapBase(){Clear();};

	mutable std::map<unsigned long, T*> m_Map;
	mutable std::list<unsigned long> m_Indices;
	mutable unsigned long m_CurIndex;

	virtual bool _Free(T*& p_Element) const
	{
		if ( !p_Element )
			return false;
		else
		{
			delete p_Element;
			p_Element = 0;
			return true;
		}
	}

	unsigned long _GetIndex() const
	{
		if ( m_Indices.empty() )
		{
			++m_CurIndex;
			return m_CurIndex;
		}
		unsigned long Index = m_Indices.back();
		m_Indices.pop_back();
		return Index;
	}
public:
	/*
		This should add a element, and return its index
			- All things that are to be placed in this container have to have a copy constructor(unless unsafe_add is used)
	*/
	virtual unsigned long Add(const T& p_NewElement) const
	{
		T* temp = new T(p_NewElement);
		unsigned long Index = _GetIndex();
		if ( m_Map.find(Index) != m_Map.end() )
		{
			_Free(temp);
			return 0;
		}
		else
			m_Map[Index] = temp;
		return Index;
	}

	/*
		This should add an element, and return its index
			- This is a unsafe function, and has such warning attached. This function can produce memory leaks if used
			incorrectly.
	*/
#ifndef CCONTAINER_NO_WARNINGS
	virtual unsigned long UNSAFE(cMapBase<T>::Add(const T&)) 
		Add(T* p_NewElement) const
#else
	virtual unsigned long
		Add(T* p_NewElement) const
#endif
	{
		unsigned long Index = _GetIndex();
		if ( m_Map.find(Index) != m_Map.end() )
		{
			return 0;
		}
		else
			m_Map[Index] = p_NewElement;
		return Index;
	}

	/*
		This should remove an element at the given index.
	*/
	virtual bool Remove(int p_Index) const
	{
		std::map<unsigned long, T*>::iterator find_iter = m_Map.find(p_Index);
		if ( find_iter == m_Map.end() )
			return false;
		else
		{
			m_Indices.push_back(p_Index);
			return _Free(m_Map[p_Index]);
		}
	}

	/*
		This should delete(free memory) of all elements.
	*/
	virtual void Clear() const
	{
		std::map<unsigned long, T*>::iterator free_iter = m_Map.begin();
		for ( /**/; free_iter != m_Map.end(); ++free_iter )
		{
			_Free((*free_iter).second);
		}
		m_Indices.clear();
		m_Map.clear();
		m_CurIndex = 0;
	}

	/*
		These should access the element at given index.
	*/
	virtual T* operator[](unsigned long p_Index) const
	{
		std::map<unsigned long, T*>::iterator find_iter = m_Map.find(p_Index);

		try
		{
			if ( find_iter == m_Map.end() )
			{
				throw (std::string("Bad index provided. cContainerBase<")+std::string(typeid(T).name())+std::string(">::operator[](unsigned long)"));
			}
		}
		catch(const std::string& str)
		{
			std::cout << "\n" << str << "\n";
			return NULL;
		}
		return (*find_iter).second;
	}
};

This is the cSingletonBase class as well

Code:

#pragma once
#include "VariaMacros.h"

template< class T >
class cSingletonBase
{
public:
    static T* Create()
    {
        if ( m_Singleton )
        {
            ++m_RefCount;
            return m_Singleton;
        }
        m_RefCount = 1;
        m_Singleton = new T;
        return m_Singleton;
    }

    static bool Free()
    {
        if ( m_Singleton )
        {
            //Bad ref count check
            if ( m_RefCount <= 0 )
                _asm int 3

            --m_RefCount;
            if ( !m_RefCount )
            {
                delete m_Singleton; m_Singleton = 0;
                return true;
            }
            return false;
        }
        return true;
    }
protected:
    cSingletonBase()
    { }
    virtual ~cSingletonBase()
    { }

private:
    DisableClassCopying(cSingletonBase);

    static T* m_Singleton;
    static unsigned int m_RefCount;
};

template< class T > T* cSingletonBase< T >::m_Singleton = 0;
template< class T > unsigned int cSingletonBase< T >::m_RefCount = 0;

What I don't quite understand is how you are going to use it. You put something into the map and receive an integer. Now you need to store that integer in some other container, so you know what is there in the cMapBase? (How does all that hashing help - especially if hash collisions are not handled adequately?)

Then, what's the point in storing dynamically allocated pointers, if you are going to make a copy anyway? Wouldn't that just add more memory fragmentation and overhead for no good reason? Also, why make _Free a (private) member function, if it could be generally useful in any context?

Another thing is exceptions: I think it is more customary to throw exception types, not invalid iterators which are of no use for the catcher.

I don't understand either, why a general-purpose container should be a singleton. Why not just make it uncopyable, if you don't bother to implement copying and assignment, and let a smart pointer, such as boost::shared_ptr take care of reference counting, should you need to share the container?

The purpose of the container is for my game.
Also, the exceptions are incomplete, just havent finished that part yet.

The reason _Free is private is because if a class is derived from the cMapBase, and say the new class is specialized for a particular type, then _Free can be overloaded to handle a special case, and as such does not have a general use anymore.

Also, what do you mean by
>>what's the point in storing dynamically allocated pointers, if you are going to make a copy anyway?

I meant this. Ok, so you can push values into it as well as pointers because of the other overloads...

Code:

    virtual unsigned long Add(const T& p_NewElement) const
    {
        T* temp = new T(p_NewElement);

The reason _Free is private is because if a class is derived from the cMapBase, and say the new class is specialized for a particular type, then _Free can be overloaded to handle a special case, and as such does not have a general use anymore.

Aha. You might also look into something like boost::shared_ptr uses. You can just give it a function pointer that is used to release the object.

I still don't get what's good about mapping objects to (practically) random integers. So I add two objects to the container and receive indices 287 and 4892. How do these indices help me know what is what?
Besides, this means that you can't Add objects that have identical bit patterns (e.g default-constructed objects), or objects whose hash value just happens to collide with that of another object?

Another thing that's hard to understand: what is so good about making a container singleton? So, I can't have, for example, a separate list of friendly creatures and hostile creatures? Also, doesn't the reference count depend on manually calling Free correctly, so in the face of exceptions the reference count won't necessarily be correct?

Ok, I couldnt make since of what you were trying to say.

I meant this. Ok, so you can push values into it as well as pointers because of the other overloads...

Code:

    virtual unsigned long Add(const T& p_NewElement) const
    {
        T* temp = new T(p_NewElement);

I don't understand what you are trying to say about that.

I just got home from work, and had a couple ideas to add to this, so here is the modded code. Probably not any better, but maybe it is.
I added a sequential Index. (Also reuses Indices)
I added a simple exception handler.
I also decided to only allow getting the data by pointer. Thus if a exception is thrown then NULL is returned.

Hopefully, someone can point out if I messed up anything.

Code:

#pragma once
#include "VariaMacros.h"
#include "cSingletonBase.h"

#include <map>
#include <list>
#include <string>

template <typename T>
class cMapBase : public cSingletonBase<cMapBase<T> >
{
	EnableSingleton(cMapBase);
	DisableClassCopying(cMapBase);
private:
	cMapBase():m_CurIndex(0){};
	virtual ~cMapBase(){Clear();};

	mutable std::map<unsigned long, T*> m_Map;
	mutable std::list<unsigned long> m_Indices;
	mutable unsigned long m_CurIndex;
	mutable unsigned long m_LastIndex;//This is the index to the last added element. Reset if Remove is called.

	virtual void _Free(T*& p_Element) const
	{
		if ( !p_Element )
			return;
		delete p_Element;
		p_Element = 0;
	}

	unsigned long _GetIndex() const
	{
		if ( m_Indices.empty() )
		{
			++m_CurIndex;
			return m_CurIndex;
		}
		m_Indices.sort();
		unsigned long Index = m_Indices.back();
		m_Indices.pop_back();
		m_LastIndex = Index;
		return Index;
	}
public:
	/*
		This should add a element, and return its index
			- All things that are to be placed in this container have to have a copy constructor(unless unsafe_add is used)
	*/
	virtual unsigned long Add(const T& p_NewElement) const
	{
		T* temp = new T(p_NewElement);
		unsigned long Index = _GetIndex();
		if ( m_Map.find(Index) != m_Map.end() )
		{
			_Free(temp);
			return 0;
		}
		else
		{
			m_Map[Index] = temp;
		}
		return Index;
	}

	/*
		This should add an element, and return its index
			- This is a unsafe function, and has such warning attached. This function can produce memory leaks if used
			incorrectly.
	*/
#ifndef CCONTAINER_NO_WARNINGS
	virtual unsigned long UNSAFE(cMapBase<T>::Add(const T&)) 
		Add(T* p_NewElement) const
#else
	virtual unsigned long
		Add(T* p_NewElement) const
#endif
	{
		unsigned long Index = _GetIndex();
		if ( m_Map.find(Index) != m_Map.end() )
		{
			return 0;
		}
		else
		{
			m_Map[Index] = p_NewElement;
		}
		return Index;
	}

	unsigned long LastAdded()
	{
		return m_LastIndex;
	}

	/*
		This should remove an element at the given index.
	*/
	virtual bool Remove(int p_Index) const
	{
		m_LastIndex = 0;
		std::map<unsigned long, T*>::iterator find_iter = m_Map.find(p_Index);
		if ( find_iter == m_Map.end() )
			return false;
		else
		{
			m_Indices.push_back(p_Index);
			_Free(m_Map[p_Index]);
			m_Map.erase(find_iter);
			return true;
		}
	}

	/*
		This should delete(free memory) of all elements.
	*/
	virtual void Clear() const
	{
		std::map<unsigned long, T*>::iterator free_iter = m_Map.begin();
		for ( /**/; free_iter != m_Map.end(); ++free_iter )
		{
			_Free((*free_iter).second);
		}
		m_Indices.clear();
		m_Map.clear();
		m_CurIndex = 0;
		m_LastIndex = 0;
	}

	/*
		These should access the element at given index.
	*/
	virtual T* operator[](unsigned long p_Index) const
	{
		std::map<unsigned long, T*>::iterator find_iter = m_Map.find(p_Index);

		try
		{
			if ( find_iter == m_Map.end() )
			{
				throw (std::string("Exception: Bad index provided.\n  -->cContainerBase<")+std::string(typeid(T).name())+std::string(">::operator[](unsigned long)"));
			}
		}
		catch(const std::string& str)
		{
			std::cout << "\n" << str << "\n";
			return NULL;
		}
		return (*find_iter).second;
	}
};

You seem to be using some MS extensions.

- Include <typeinfo> to use typeid.

- All the iterator declarations should be preceeded by typename, since they are dependent types. (A typedef of the iterator/const_iterator types might help making code more pleasant to the eyes.)

- const correctness or the lack thereof. I can't understand why you have made all the members mutable and then declare all methods that clearly should and do modify the state of the containers const. Perhaps _GetIndex alone might be logically const and the related members mutable, but apparently you'd only call it from logically non-const methods anyway.
Ironically, the only non-const method is LastAdded, which is also logically the only const method of this class.

- The LastAdded method doesn't work at all, since m_LastIndex is nowhere updated to some meaningful value.

- Usage of exceptions. It makes zero sense to throw and catch the exception in the same method and then return NULL. Exceptions are thrown so that callers of the method somewhere up the call stack can decide what to do about the error. You could just print the message there (again functions should do work and not print error messages really, leaving error response to the caller).
Also, it is good to use real exception types (derivates of std::exception or user-defined classes inherited from them), so that it is possible to catch unhandled exceptions at the highest level and still get some idea what's it about.
Normally, the type of the exception carries a large part of the information about the error. You shouldn't need to parse the (non-standardized) error messages to find out, for example, whether a memory allocation failed or you accessed an out-of-bounds index - you find that out by catching std::bad_alloc or std::out_of_range.

- This far the container lacks any means of iterating over its contents (in a way that would be compatible with the STL).

- I still don't quite understand the purpose of this container. Why is it good that you should have only one global instance of the container for a given type? What is it meant to be used for to make up for the overhead of every method (and the awkwardness of the user code - who apparently needs another container to store the keys and has to keep that in sync with cMapBase in face of Remove and Clear)? Perhaps you could post an intended usage description or some user code demonstrating the usage?

- Regarding the macros. Another way to make a class non-copyable is to derive it from an empty base class that just declares a private copy constructor and assignment operator. The added benefit should be that friends wouldn't be able to access this functionality either, giving you a compile-time error instead of linker error. (Or just derive from boost::noncopyable.)

This class is meant to only have one for each type. Simply because in my view there is only need for say 1 texture manager, 1 mesh manager, 1 script manager. I will remove the const on the functions. Don't ask why I put them there, as honestly I have no idea. Singleton's are in my opinion easier to keep track of. Well, I will work on the exception part, I haven't ever really used them in my classes so a little learning to do yet.

Also, m_LastAdded is updated in the _GetIndex function right before the return. It is reset with Remove() because if the last element is removed then it can't possibly still be correct.

I do not see a need for iterating of the contents of the container. Why would I need it.
This is how it should be used

Code:

int main()
{
  cMapBase<int>& testmap = *cMapBase<int>::Create();

  int Index = test.Add(321); //Get index to the value (321)
  int Size = *test[Index]; // Access the value (321)
  test.Remove(Index); //Free and remove the element (321)

  while ( !cMapBase<int>::Free() )
   ;
  return 0;
}

Also, I noticed some incorrect parts in the code. Such as in the Add() functions, the Index is received, but even if it is not used the index is still treated as if it exists. So, I will work on it all more when I get home from work, and will post my progress thus far.

Well, I did this really fast, but I removed the mutable, fixed the const-ness, fixed the m_LastIndex thing I believe, the Index leak that was in the add functions, and maybe to exception thing is right now. I just put std:ut_of_range for the sake of simplicity.

Code:

#pragma once
#include "VariaMacros.h"
#include "cSingletonBase.h"

#include <map>
#include <list>
#include <string>
#include <exception>

template <typename T>
class cMapBase : public cSingletonBase<cMapBase<T> >
{
	EnableSingleton(cMapBase);
	DisableClassCopying(cMapBase);
private:
	cMapBase():m_CurIndex(0),m_LastIndex(0),m_MemUsage(0){};
	virtual ~cMapBase(){Clear();};

	std::map<unsigned long, T*> m_Map;
	std::list<unsigned long> m_Indices;
	unsigned long m_CurIndex;
	unsigned long m_LastIndex;//This is the index to the last added element. Reset if Remove is called.
	unsigned long m_MemUsage;

	virtual void _Free(T*& p_Element) const
	{
		if ( !p_Element )
			return;
		delete p_Element;
		p_Element = 0;
	}

	unsigned long _GetIndex()
	{
		if ( m_Indices.empty() )
		{
			++m_CurIndex;
			m_LastIndex = m_CurIndex;
			return m_CurIndex;
		}
		m_Indices.sort();
		unsigned long Index = m_Indices.back();
		m_Indices.pop_back();
		m_LastIndex = Index;
		return Index;
	}
public:
	/*
		This should add a element, and return its index
			- All things that are to be placed in this container have to have a copy constructor(unless unsafe_add is used)
	*/
	virtual unsigned long Add(const T& p_NewElement)
	{
		unsigned long Index = _GetIndex();
		if ( m_Map.find(Index) != m_Map.end() )
		{
			m_Indices.push_back(Index);
			return 0;
		}
		else
		{
			
			m_MemUsage += sizeof(p_NewElement);
			T* temp = new T(p_NewElement);
			m_Map[Index] = temp;
		}
		return Index;
	}

	/*
		This should add an element, and return its index
			- This is a unsafe function, and has such warning attached. This function can produce memory leaks if used
			incorrectly.
	*/
#ifndef CCONTAINER_NO_WARNINGS
	virtual unsigned long UNSAFE(cMapBase<T>::Add(const T&)) 
		Add(T* p_NewElement)
#else
	virtual unsigned long
		Add(T* p_NewElement)
#endif
	{
		unsigned long Index = _GetIndex();
		if ( m_Map.find(Index) != m_Map.end() )
		{
			m_Indices.push_back(Index);
			return 0;
		}
		else
		{
			m_MemUsage += sizeof(*p_NewElement);
			m_Map[Index] = p_NewElement;
		}
		return Index;
	}

	unsigned long LastAdded() const
	{
		return m_LastIndex;
	}

	unsigned long GetMemoryUsage() const
	{
		return m_MemUsage;
	}
	/*
		This should remove an element at the given index.
	*/
	virtual bool Remove(int p_Index)
	{
		m_LastIndex = 0;
		std::map<unsigned long, T*>::iterator find_iter = m_Map.find(p_Index);
		if ( find_iter == m_Map.end() )
			return false;
		else
		{
			m_MemUsage -= sizeof((*find_iter).second);
			m_Indices.push_back(p_Index);
			_Free(m_Map[p_Index]);
			m_Map.erase(find_iter);
			return true;
		}
	}

	/*
		This should delete(free memory) of all elements.
	*/
	virtual void Clear()
	{
		std::map<unsigned long, T*>::iterator free_iter = m_Map.begin();
		for ( /**/; free_iter != m_Map.end(); ++free_iter )
		{
			_Free((*free_iter).second);
		}
		m_Indices.clear();
		m_Map.clear();
		m_CurIndex = 0;
		m_LastIndex = 0;
		m_MemUsage = 0;
	}

	/*
		These should access the element at given index.
	*/
	virtual T* operator[](unsigned long p_Index)
	{
		std::map<unsigned long, T*>::iterator find_iter = m_Map.find(p_Index);
		if ( find_iter == m_Map.end() )
		{
			throw std::out_of_range("Invalid index was provided to cMapBase<typename T>::operator[](unsigned long)");
		}
		return (*find_iter).second;
	}
};

Well, I am off to work, so I will check back in about 4-6 hours. Thank you for any assistance you are able to provide.