[C++] Issue with member function corrupting data member

Started by
4 comments, last by ApochPiQ 15 years, 3 months ago
Hey. I have two classes that hold a Surface_Cache data member. One has a function Cache that returns a reference to the data member, and one doesn't. The one that doesn't have the function works fine; ie the data member is OK upon and after construction, but the one that does isn't OK. The internal Handle_Manager (Surface_Cache derives from Handle_Manager) has junk values.
#include <exception>

#include "vld.h"

#include "SDL.h"

#include "Game_Settings.h"
#include "SDL_System.h"

#include "Test_Game.h"

#include "Wrapper.h"

int main(int argc, char* argv[])
{
	try
	{
		// Initialise SDL.
		SDL_System sdl(SDL_INIT_EVERYTHING);
		// Initialise application settings.
		Settings::Initialise("settings.xml");
		
		/* testing */
		Wrapper wrapper;
		Wrapper2 wrapper2;
		/* end testing */
		
		// Initialise game.
		Test_Game game;
		// Run game until user exits.
		game.Run();
	}
	catch(const std::exception& std_e)
	{
		std::cout << std_e.what();
	}

	return 0;
}

class Wrapper
{
public:
	Wrapper();
	~Wrapper();
private:
	Surface_Cache cache;
};

class Wrapper2
{
public:
	Wrapper2();
	~Wrapper2();

	Surface_Cache& Cache();
private:
	Surface_Cache cache;
};

Wrapper::Wrapper()
: cache(SDL_Tools::Colours::WHITE)
{
}

Wrapper::~Wrapper()
{
}

Wrapper2::Wrapper2()
: cache(SDL_Tools::Colours::WHITE)
{
}

Wrapper2::~Wrapper2()
{
}


Surface_Cache& Wrapper2::Cache()
{
	return cache;
}

This is the Surface_Cache:
class Surface_Cache : public Handle_Manager<std::string>
{
public:
	typedef boost::unordered_map<int, SDL_Surface*> surface_cache;
	typedef surface_cache::iterator cache_it;
	
	Surface_Cache(SDL_Colour default_colour_key);
	// Returns the surface at hash. Throws if non-existant.
	SDL_Surface* Surface_At(const Handle& handle);
private:
	// Loads a surface from file_name.
	SDL_Surface* Load_Surface(const std::string& file_name,	SDL_Colour* colour_key);
	// Loads multiple surfaces from file_name.
	//void Load_Surfaces(const std::string& file_name);

	surface_cache cache;
	SDL_Colour default_colour_key;
};

Surface_Cache::Surface_Cache(SDL_Colour default_colour_key)
: default_colour_key(default_colour_key)
{
}

The Handle_Manager class:
template<typename T>
class Handle_Manager;

template<typename T>
class Handle_Impl
{
public:
	Handle_Impl() : id(0), owner(NULL) {}
	Handle_Impl(int id, Handle_Manager<T>& owner) : id(id), owner(&owner) {}

	const T& operator*()
	{
		// Do I have to ensure that value (optional) is initialised?
		return (owner->Get(id));
	}

	Handle_Impl& operator=(Handle_Impl& rhs)
	{
		id = rhs.id;
		owner = rhs.owner;

		return *this;
	}

	int id;
	Handle_Manager<T>* owner;
};

template<typename T>
class Handle_Manager
{
public:
	typedef typename boost::shared_ptr<Handle_Impl<T> > Handle;
	typedef typename boost::weak_ptr<Handle_Impl<T> > Weak_Handle;

	Handle_Manager() {}
	virtual ~Handle_Manager() {}

	/* Get the handle that corresponds to value (if it exists). */
	virtual Handle Get(const T& value)
	{ 
		/* Get the handle at the index (hash) of value. */
		hash_map_it it = hash_table.find(value);
		if(it != hash_table.end())
		{
			Handle handle = handles[it->second].lock();
			return handle;
		}
		/* Handle doesn't exist; create entry for value in handles, values and hash table. */
		return Add(value);
	}

	/* Get the value that corresponds to handle (if it exists). */
	T Get(const Handle& handle)
	{
		return *values[handle->id];
	}
protected:
	// Find the first free index in handles.values and return it.
	// If there is no free index, let caller know with optional.
	boost::optional<int> Find_Free_Index()
	{
		int i = 0;
		int size = values.size();
		while(i < size && values != 0) ++i;
		
		if(i < size)
		{ // There is a free index; return it.
			return boost::optional<int>(i);
		}
		// There is no free index; caller has to push_back.
		return boost::optional<int>();
	}

	// Adds a handle for value.
	// Returns added handle.
	virtual Handle Add(const T& value)
	{
		boost::optional<int> index = Find_Free_Index();
		Handle handle;
		if(index)
		{ // Free index exists; use it.
			handle = Handle(new Handle_Impl<T>(*index, *this));
			handles[*index] = handle;
			values[*index] = value;
			hash_table.insert(hash_map_pair(value, handle->id));
		}
		else
		{ // No free index exists; push back.
			handle = Handle(new Handle_Impl<T>(handles.size(), *this));
			handles.push_back(handle);
			values.push_back(value);
			hash_table.insert(hash_map_pair(value, handle->id));
		}
		return handle;
	}

	virtual void Clean()
	{
		for(unsigned int i = 0; i < values.size(); ++i)
		{
			if(handles.expired())
			{ // Handle (shared_ptr) expired; clean its respective entries up.
				// Clean handle entry.
				handles.reset();
				hash_map_it it = hash_table.find(*values);
				if(it != hash_table.end())
				{ // Clean hash map entry.
					hash_table.erase(it);
				}
				// Clean value entry.
				values = boost::optional<T>();
			}
		}
	}

	// Stores weak handles to allow clean to determine when to destroy the shared_ptr.
	std::vector<Weak_Handle> handles;
	// Stores the values of each handle to allow the hash -> value association.
	std::vector<boost::optional<T> > values;
	// Stores the value -> hash associations.
	typedef boost::unordered_map<T, int> hash_map;
	typedef typename hash_map::iterator hash_map_it;
	typedef std::pair<T, int> hash_map_pair;
	hash_map hash_table;
};

Does anyone know why this would happen? I've had a similar problem here that still isn't solved. Cheers.

Advertisement
Sorry nothing wrong stands out to me. Maybe you could try to isolate the problem and provide a working sample exhibiting it. Also, you said "junk values" but showing those values and how you get them, and when, and after doing what, would help too.

regards
If you declare the Wrapper2 instance before the Wrapper instance, do the symptoms change?
You have a small mistake here, in the Surface_Cache implementation:

	SDL_Colour default_colour_key;};Surface_Cache::Surface_Cache(SDL_Colour default_colour_key): default_colour_key(default_colour_key){}


The member function and the parameter have the same name. Now, what is going to happen is the compiler will come along, notice that you are initializing default_colour_key, and look at what value you want to initialize it to. Except when it does the name lookup, it's going to find the member default_colour_key instead of the parameter to the function.

(Someone check me on this, but AFAIK this is undefined behaviour land.)


Renaming the member, the function parameter, or both should fix the problem.

[edit] I am incorrect; see below posts.

[Edited by - ApochPiQ on January 10, 2009 5:56:14 PM]

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

For initialiser lists it is perfectly legal to use a parameter with the same name as a member. You do have to be careful however, as a small typo in the parameter name will initialise the member with itself...
Well I'll be damned - it does seem to work (tested in VS2005). I'd still recommend renaming one or both of the variables though to avoid confusion [smile]

Mybowlcut - is this the exact code you were running? What's your procedure for debugging, and how are you observing these junk values?

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

This topic is closed to new replies.

Advertisement