6 Replies - 902 Views - Last Post: 22 February 2013 - 11:29 AM Rate Topic: -----

#1 CodingDesire  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 21-December 09

Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 09:49 AM

Alright so I've been struggling to find the error but I simply can't. Given the following code:
#include <iostream>
#include <vector>

enum ScriptType
{
	SCRIPT_TYPE_CREATURE_SCRIPT,
	SCRIPT_TYPE_PLAYER_SCRIPT
};

class Script
{
private:
	ScriptType type;
public:
	Script(ScriptType type) : type(type)
	{
	}

	ScriptType getScriptType() const
	{
		return type;
	}
};

class CreatureScript : public Script
{
public:
	CreatureScript() : Script(SCRIPT_TYPE_CREATURE_SCRIPT)
	{
	}

	virtual void OnDealDamage() { }
	virtual void OnKill() { }
};

class PlayerScript : public Script
{
public:
	PlayerScript() : Script(SCRIPT_TYPE_PLAYER_SCRIPT)
	{
	}

	virtual void OnSay() { }
};

class ScriptFactory
{
private:
	static std::vector<script *> scripts;
public:
	static void Register(Script *s);
	static void Unregister();
	
	static std::vector<script *> getScriptsByType(ScriptType type);
};

std::vector<script *> ScriptFactory::scripts;

void ScriptFactory::Register(Script *s)
{
	scripts.push_back(s);
	std::cout << "Pushed at address: " << s << std::endl;
}

void ScriptFactory::Unregister()
{
	std::vector<script *>::iterator iter = scripts.begin();
	for (iter; iter != scripts.end(); ++iter)
	{
		std::cout << "Trying to free memory at: " << *iter << std::endl;
		delete *iter;
	}

	scripts.clear();
}

std::vector<script *> ScriptFactory::getScriptsByType(ScriptType type)
{
	std::vector<script *>::iterator iter = scripts.begin();
	std::vector<script *> values;
	for (iter; iter != scripts.end(); ++iter)
		if ((*iter)->getScriptType() == type)
			values.push_back(*iter);

	return values;
}

class CreatureScriptTest : public CreatureScript
{
public:
	CreatureScriptTest() : CreatureScript()
	{
	}

	void OnDealDamage()
	{
		std::cout << "OnDealDamage() called from CreatureScriptTest." << std::endl;
	}
};

class CreatureScriptTest2 : public CreatureScript
{
public:
	CreatureScriptTest2() : CreatureScript()
	{
	}

	void OnDealDamage()
	{
		std::cout << "OnDealDamage() called from CreatureScriptTest2." << std::endl;
	}
};

class PlayerScriptTest : public PlayerScript
{
public:
	PlayerScriptTest() : PlayerScript()
	{
	}

	void OnSay()
	{
		std::cout << "OnSay() called from PlayerScriptTest." << std::endl;
	}
};

class ScriptMgr
{
	typedef std::vector<script *> ScriptStorage;
#define ITERATE(container, type) \
	ScriptStorage scripts = ScriptFactory::getScriptsByType(type); \
	for (ScriptStorage::iterator itr = container.begin(); itr != container.end(); ++itr)
public:
	static void OnCreatureDealDamage()
	{
		ITERATE(scripts, SCRIPT_TYPE_CREATURE_SCRIPT)
			static_cast<CreatureScript *>(*itr)->OnDealDamage();
	}

	static void OnCreatureKill()
	{
		ITERATE(scripts, SCRIPT_TYPE_CREATURE_SCRIPT)
			static_cast<CreatureScript *>(*itr)->OnKill();
	}

	static void OnPlayerSay()
	{
		ITERATE(scripts, SCRIPT_TYPE_PLAYER_SCRIPT)
			static_cast<PlayerScript *>(*itr)->OnSay();
	}
};

int main()
{
	ScriptFactory::Register(new CreatureScriptTest());
	ScriptFactory::Register(new CreatureScriptTest2());
	ScriptFactory::Register(new PlayerScriptTest());
	ScriptMgr::OnCreatureDealDamage();
	ScriptMgr::OnPlayerSay();
	
	ScriptFactory::Unregister();

	return 0;
}


It gives the correct output and everything seems to be working fine, except that I get a runtime error on ScriptFactory::Unregister() at line 71: delete *iter;
Using MSVC 2010 I get the following on running in Debug mode:
CLICK
So I see that the program is trying to free a valid memory address which was previously allocated using new. But I still get that error telling me that the pointer is invalid.

Is This A Good Question/Topic? 0
  • +

Replies To: Error on freeing memory from a vector of pointers

#2 jimblumberg  Icon User is online

  • member icon

Reputation: 5336
  • View blog
  • Posts: 16,616
  • Joined: 25-December 09

Re: Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 10:35 AM

Before I can run your program you may want to see about fixing some of these errors.

Quote

main.cpp||In constructor ‘Script::Script(ScriptType)’:|
main.cpp|15|warning: declaration of ‘type’ shadows a member of 'this' [-Wshadow]|
main.cpp|49|error: ‘script’ was not declared in this scope|
main.cpp|49|error: template argument 1 is invalid|
main.cpp|49|error: template argument 2 is invalid|
main.cpp|54|error: ‘script’ was not declared in this scope|
main.cpp|54|error: template argument 1 is invalid|
main.cpp|54|error: template argument 2 is invalid|
main.cpp|57|error: ‘script’ was not declared in this scope|
main.cpp|57|error: template argument 1 is invalid|
main.cpp|57|error: template argument 2 is invalid|
main.cpp|57|error: invalid type in declaration before ‘;’ token|
main.cpp|61|error: request for member ‘push_back’ in ‘ScriptFactory::scripts’, which is of non-class type ‘int’|
main.cpp|67|error: ‘script’ was not declared in this scope|
main.cpp|67|error: template argument 1 is invalid|
main.cpp|67|error: template argument 2 is invalid|
main.cpp|67|error: expected initializer before ‘iter’|
main.cpp|68|error: ‘iter’ was not declared in this scope|
main.cpp|68|error: request for member ‘end’ in ‘ScriptFactory::scripts’, which is of non-class type ‘int’|
main.cpp|74|error: request for member ‘clear’ in ‘ScriptFactory::scripts’, which is of non-class type ‘int’|
main.cpp|77|error: ‘script’ was not declared in this scope|
main.cpp|77|error: template argument 1 is invalid|
main.cpp|77|error: template argument 2 is invalid|
main.cpp|79|error: ‘script’ was not declared in this scope|
main.cpp|79|error: template argument 1 is invalid|
main.cpp|79|error: template argument 2 is invalid|
main.cpp|79|error: expected initializer before ‘iter’|
main.cpp|80|error: template argument 1 is invalid|
main.cpp|80|error: template argument 2 is invalid|
main.cpp|80|error: invalid type in declaration before ‘;’ token|
main.cpp|81|error: ‘iter’ was not declared in this scope|
main.cpp|81|error: request for member ‘end’ in ‘ScriptFactory::scripts’, which is of non-class type ‘int’|
main.cpp|83|error: request for member ‘push_back’ in ‘values’, which is of non-class type ‘int’|
main.cpp|129|error: ‘script’ was not declared in this scope|
main.cpp|129|error: template argument 1 is invalid|
main.cpp|129|error: template argument 2 is invalid|
main.cpp|136|error: invalid use of qualified-name ‘::iterator’|
main.cpp|136|error: expected ‘;’ before ‘itr’|
main.cpp|136|error: ‘itr’ was not declared in this scope|
main.cpp|136|error: request for member ‘begin’ in ‘scripts’, which is of non-class type ‘ScriptMgr::ScriptStorage {aka int}’|
main.cpp|136|error: request for member ‘end’ in ‘scripts’, which is of non-class type ‘ScriptMgr::ScriptStorage {aka int}’|
main.cpp|136|error: expected ‘)’ before ‘;’ token|
main.cpp|136|error: ‘itr’ was not declared in this scope|
main.cpp|136|error: expected ‘;’ before ‘)’ token|
main.cpp|142|error: invalid use of qualified-name ‘::iterator’|
main.cpp|142|error: expected ‘;’ before ‘itr’|
main.cpp|142|error: ‘itr’ was not declared in this scope|
main.cpp|142|error: request for member ‘begin’ in ‘scripts’, which is of non-class type ‘ScriptMgr::ScriptStorage {aka int}’|
main.cpp|142|error: request for member ‘end’ in ‘scripts’, which is of non-class type ‘ScriptMgr::ScriptStorage {aka int}’|
main.cpp|142|error: expected ‘)’ before ‘;’ token|
main.cpp|142|error: ‘itr’ was not declared in this scope|
main.cpp|142|error: expected ‘;’ before ‘)’ token|
||More errors follow but not being shown.|
||Edit the max errors limit in compiler options...|
||=== Build finished: 50 errors, 1 warnings ===|


Next why are you trying to call Unregister() on memory the class didn't allocate. This memory was allocated in main() and delete should be called not Unregister().

Jim
Was This Post Helpful? 0
  • +
  • -

#3 CodingDesire  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 21-December 09

Re: Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 10:38 AM

The code compiles just fine on MSVC 2010.
Why not on Unregister? The adresses that I allocate in main() are pushed into a vector that's part of ScriptFactory (see Register). Unregister frees (or should free) the addresses stored in the vector.

PS: I see why it doesn't compile, it's because of code tags, looks like < Script > gets converted to < script > for some reason, if a mod can correct this ...

This post has been edited by CodingDesire: 22 February 2013 - 10:42 AM

Was This Post Helpful? 0
  • +
  • -

#4 jimblumberg  Icon User is online

  • member icon

Reputation: 5336
  • View blog
  • Posts: 16,616
  • Joined: 25-December 09

Re: Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 10:51 AM

First how can this code compile C/C++ is case sensitive so the following should produce an error:
	static std::vector<script *> scripts;

Where have you defined a class/structure/typedef with the name of script. Your class is named Script.

You should be deleting the memory in your ScriptFactory destructor.

Edit: Something like:
   ~ScriptFactory() {
   for(int i = 0; i < ScriptFactory::scripts.size(); ++i)
	{
		std::cout << "Trying to free memory at: " << ScriptFactory::scripts[i] << std::endl;
      delete ScriptFactory::scripts[i];
	}
}


Jim

This post has been edited by jimblumberg: 22 February 2013 - 10:55 AM

Was This Post Helpful? 1
  • +
  • -

#5 CodingDesire  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 21-December 09

Re: Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 11:06 AM

Quote

Where have you defined a class/structure/typedef with the name of script. Your class is named Script.

I know but Dreamincode code tags renamed it to "script" lowercase for some reason.

EDIT: No that wouldn't work, just realized right now. Destructor never gets called because I'm not creating any instance of ScriptFactory, because all I have is a bunch of static methods, and I call them without creating an object.

This post has been edited by CodingDesire: 22 February 2013 - 11:10 AM

Was This Post Helpful? 0
  • +
  • -

#6 jimblumberg  Icon User is online

  • member icon

Reputation: 5336
  • View blog
  • Posts: 16,616
  • Joined: 25-December 09

Re: Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 11:22 AM

Even though they're static doesn't mean they don't belong to the class.


Also this doesn't get rid of all the memory leaks, it just allows the program to run. You will still have about 24 bytes of unreleased memory.

Here is what Valgrind reports:

Quote

main.cpp||In function 'main' :|
main.cpp|165|8 bytes in 1 blocks are definitely lost in loss record 1 of 3|
main.cpp||In function 'main' :|
main.cpp|166|8 bytes in 1 blocks are definitely lost in loss record 2 of 3|
main.cpp||In function 'main' :|
main.cpp|167|8 bytes in 1 blocks are definitely lost in loss record 3 of 3|

The lines in question are:

	ScriptFactory::Register(new CreatureScriptTest());
	ScriptFactory::Register(new CreatureScriptTest2());
	ScriptFactory::Register(new PlayerScriptTest());



Why are you trying to use static in this class?

Jim
Was This Post Helpful? 0
  • +
  • -

#7 CodingDesire  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 21-December 09

Re: Error on freeing memory from a vector of pointers

Posted 22 February 2013 - 11:29 AM

It's just that I've presented an example, but the actual project is way bigger, and contains more script types, etc, that are spread multiple source files and header files, so to make things easier and not bother with an instance of ScriptFactory (which I really don't need), I simply used static methods. I suppose I could use the Singleton pattern, but I've already tried, same error (this time I was calling "delete" in deconstructor).

PS: And read my 3rd post. The reason Valgrid reports the memory leaks is because the Deconstructor never gets called (I've explained why), so the memory for the three pointers never gets freed.
And I also don't think the error has something to do with the fact that I use static methods.

This post has been edited by CodingDesire: 22 February 2013 - 11:39 AM

Was This Post Helpful? 0
  • +
  • -

Page 1 of 1