I want to understand this pointer error.

Question

1.00/5 (1 vote)

See more:

The code is here. It's almost 1000 lines. The first 800 lines are just class and declaration. The problem start at around line 910+.

http://cpp.sh/7qrh[^]

I have this code here, it compiles ok.
But it crashes when executing pop_back() on the vector in line 919.
It crashes and show the memory header file (which I don't know how to read).
The error is 'pointer being freed was not allocated'.

I'm still learning and didn't know much about debugging.

This is some lines from the code.

C++

template <typename A, typename B>
void battle(vector<A> &human, vector &zombie){

	while (!human.empty() || !zombie.empty()){
		human.pop_back();
	}
}

In main, I have this.

C++

vector<unique_ptr<Warrior>> warriors;
vector<unique_ptr<Knight>>  knights;
vector<unique_ptr<Goblin>>  goblins;
vector<unique_ptr<Orc>>     orcs;
battle(warriors, goblins);

I just want to understand why this happen?
What can I do to improve my code?

Update:
new code : http://cpp.sh/4pcg[^]

Posted 17-Dec-15 5:27am

Are Riff

Updated 17-Dec-15 8:54am

v3

Add a Solution

2 solutions

Solution 1

No idea. But then, I'm not going to go to a random site and look at 1000 lines of code, counting them until I find "line 910" and then guessing which part of that is giving you a problem.

The error message is pretty explicit: "pointer being freed was not allocated" - which means that either you didn't allocate any memory for an object, or you corrupted the pointer somewhere in those 1000 lines of code.

And I can't run it to find out, because I have no idea what it does, or is meant to do, or anything about it. Even if I wanted to, which frankly, I don't.

So get busy with the debugger and start finding out what is being freed, and what value it actually contains, then start looking for what it should contain, and why it doesn't.

Posted 17-Dec-15 5:35am

OriginalGriff

Comments

Are Riff 17-Dec-15 12:51pm

The problem didn't occur if I do push_back() within main() itself. It only happen when I move it inside a templated function.
template <typename b="" typename="" a,="">
void battle(vector &human, vector<b> &zombie){

while (!human.empty() || !zombie.empty()){
human.pop_back();
}
}

I initialize the vector like this:
vector<unique_ptr<warrior>> warriors;
vector<unique_ptr<knight>> knights;
vector<unique_ptr<goblin>> goblins;
vector<unique_ptr<orc>> orcs;

should I not use unique pointer then?

Are Riff 17-Dec-15 12:55pm

I updated the question. Hope it helps.

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Andreas Gieriet · Accepted Answer · 2015-12-17T07:25:00

Solution 2

See the definition of pop_back().
E.g. from http://www.cplusplus.com/reference/vector/vector/pop_back/[^]. It says that pop_back() from an empty collection is undefined behavior, which includes "crashing".

[EDIT]
Your logic is broken: while(!human.empty() || !zombie.empty()) ... is also true if human is empty and zombie is not empty. In that case you try to pop from human even it might be empty.
Remove the zombie part and all is fine.
[/EDIT]

Cheers
Andi

Posted 17-Dec-15 7:25am

Andreas Gieriet

Updated 17-Dec-15 7:31am

v4

Comments

Are Riff 17-Dec-15 13:57pm

Thanks, I'll look it up.
I also think that deque would be better since it is easier to access and pop the first element.
I also removes all the smart pointers and use basic deque because someone commented that STL containers put their elements on the heap anyway.

Andreas Gieriet 17-Dec-15 14:07pm

Be careful!
C++ makes clear statements on performance - no guessing.
Removing from front in a vector is bad: a vector is kind of a dynamic array and it is best added/removed in the back, not the front or in between.
E.g. see http://www.cplusplus.com/reference/vector/vector/ and http://www.cplusplus.com/reference/algorithm/.
Check for the "complexity" sections of each function.
Cheers
Andi
PS: In your code, the logic is wrong! Fix that first before contemplating on "weird" optimizations...

Are Riff 17-Dec-15 14:51pm

Thanks for your feedback. I really appreciate it.
I should've use deque from the beginning.
I got it to work. The logic also have been fixed.
Take a look.
http://cpp.sh/4pcg

Can you comment a bit on my code. What can I do to improve it?
note: a lot of other code such as the operators are from initial testing when I first created those classes. Is it OK to leave it there?

Andreas Gieriet 17-Dec-15 16:23pm

Hm, if you want my honest answer? Rejected.
1) Far too much copy-paste code.
All classes have the same structure but only slightly varying "weights"...
2) Far too much dead code.
If operators are not used, remove all that code.
3) Stream handling is weird/broken.
E.g. you pass an ostream parameter but write in the function
to cout instead...
My estimation: the whole functionality could be coded in 5% of the code.
I.e. 95% of the code is waste or copy-paste.

This code in not maintainable.
"Less" would be "more".

I know, this is a harsh comment...
Regards
Andi

Are Riff 17-Dec-15 16:41pm

Nah it's ok. I expected that.
Originally, I did class inheritance from two base classes, but face some linkers error (my bad). So I just put that inheritance idea aside for a while and copy away those classes.

I will review my code and try to shortened it.
Thanks for the honest answer.
sigh.. I still have lots to learn.

Andreas Gieriet 17-Dec-15 17:17pm

If you show me the linker error I might show you a solution for it. ;-)
Cheers
Andi

Are Riff 17-Dec-15 21:52pm

Able to trim down the 1k lines of code to just 125 retaining the core functions.
http://cpp.sh/7uxk5
Can I get new honest comment?
Please, I'm learning.

Andreas Gieriet 18-Dec-15 1:41am

Here my quick review findings:
1) signature in randomize functions: no need for references.
E.g. double randomize(double minVal, double maxVal) ...
2) Make the global randomize functions protected members of Fighter.
3) Too extensive use of auto: make functions return a concrete type
E.g. getHealth returns a double
4) void newBattle(deque<a> &goodSide, deque<b> &badSide): no need for templates
E.g. void newBattle(deque<fighter> &goodSide, deque<fighter> &badSide) ...
5) void GenerateArmy(deque<T> &wars, const int &number, const char &strength): must be template (since you create an instance of T in the function) but the deque is fine if concrete
E.g. void GenerateArmy(deque<fighter> &wars, const int &number, const char &strength)...
If you do not intend to create a class hierarchy with Fighter, you may remove the template and construct a Fighter within the function instead of T(...): Fighter(...).
Cheers
Andi

Are Riff 18-Dec-15 3:30am

Thank you :-)

Sergey Alexandrovich Kryukov 17-Dec-15 15:52pm

Well spotted, a 5.
—SA

Andreas Gieriet 17-Dec-15 16:09pm

Thanks for your 5, Sergey!
Cheers
Andi