New To C++, can't figure out an error

Call Me Jimmy

Hey, I'm new to C++ and have this bit of code, it keeps giving me an "Access Violation error" and when I debug it stops at where I try to print out a variable: (I have highlighted where debug stops)

//TEAM CLASS---------------------------------

#ifndef TEAM_H
#define TEAM_H
#include <string>
using std::string;
class Team{
public:
	string name;
	int points;
	int gamesPlayed;
	int goalsFor;
	int goalsAgainst;
	int goalDifference;


	Team();
	Team(string tName);
	void setPoints(int n);
	void setGamesPlayed(int n);
	void setGoalsFor(int n);
	void setGoalsAgainst(int n);
	void updateGoalDifference();

	void displayInfo();
};

#endif




#include "stdafx.h"
#include "team.h"
#include <string>
#include <iostream>

using std::cout;

Team::Team(){}
// Initialise a new team
Team::Team(string tName){
	name = tName;
	points = 0;
	gamesPlayed = 0;
	goalsFor = 0;
	goalsAgainst = 0;
	goalDifference = 0;
}

// Setting methods
void Team::setPoints(int n){points = n;}
void Team::setGamesPlayed(int n){gamesPlayed = n;}
void Team::setGoalsFor(int n){goalsFor = n;}
void Team::setGoalsAgainst(int n){goalsAgainst = n;}

// Determines goal difference
void Team::updateGoalDifference(){
	goalDifference = goalsFor-goalsAgainst;
}


// Displays this team's info on-screen.
void Team::displayInfo(){
	cout<<name;  <B>//This is where debug points to an error.</B>
}

// LEAGUE TABLE CLASS------------------------------------

#ifndef LEAGUETABLE_H
#define LEAGUETABLE_H

#include "stdafx.h"
#include "team.h"
#include <iostream>
#include <string>
using std::string;
using std::cout;

struct entry{
	Team team;
	entry *next;

};

class LeagueTable{
	entry *first;
public:
	LeagueTable();
	LeagueTable(Team *t);
	void add(Team *t);
	entry* getFirstEntry();
	bool isEmpty();
	void printAll();
};
#endif

#include "stdafx.h"
#include "leaguetable.h"
#include <iostream>
LeagueTable::LeagueTable(Team *t){
	entry temp;

	temp.team = *t;
	temp.next = NULL;
	first = &temp;
}

LeagueTable::LeagueTable(){
	first = NULL;
}

entry* LeagueTable::getFirstEntry(){
	return first;
}
//======================================================
// Adds a new entry to the end of the league table
//======================================================
void LeagueTable::add(Team *t){
	entry *current;
	entry newEntry;
	
	//if empty, create the first entry
	if(isEmpty()){
		newEntry.team = *t;
		newEntry.next = NULL;
		first = &newEntry;
	}
	//otherwise keep traversing until the last 
	//entry is reached
	else{
		current = first;
		while(current->next !=NULL){
			current = current->next;
		}
		newEntry.team = *t;
		newEntry.next = NULL;
		current->next = &newEntry;
	}
}

//=========================================
// Checks if the league table is empty
//=========================================
bool LeagueTable::isEmpty(){
	if(first == NULL){
		return true;

	}else {
		return false;
	}
}

//============================================
// Prints all the entries of the league table
// to the standard output.
//============================================
void LeagueTable::printAll(){
	entry *current;
	entry newEntry;
	
	//if empty, create the first entry
	if(!isEmpty()){
		current = first;
		first->team.displayInfo();  // This is where the error producing code is called

		//while(current->next !=NULL){
		//	current = current->next;
		//	current->team.displayInfo();
		//}
	}
}

MAIN CLASS--------------------------------
#include "stdafx.h"
#include "team.h"
#include "leaguetable.h"
#include <iostream>
void main(){
	Team t = Team("Man Utd");
	Team* pt = &t;
	Team c = Team("Chelsea");
	LeagueTable l = LeagueTable(&c);
	//l.add(pt);
	l.printAll();
}

Ok I would greatly appreciate any help, i know there is a good bit of code but I have been trying for days to figure out the problem. I tried making the 'name' variable public and that didn't work.

satchmo

Does nobody use code tags any more?!

In the LeagueTable(Team*) constructor, the last line should be 'first = t;' and not 'first = &t;'. The way you have it at the moment, it's assigned the address of the pointer variable itself and not the address that's being pointed to. So when you try and access it, you're not accessing the address of the team, you're accessing the address of the pointer that points to the team. That's where your access violation is coming from.

Having a quick look through your code, I'd say you'll have problems with adding the address of local variables in your LeagueTable::add(Team*) function. You're declaring a local 'newEntry' variable, assigning a couple of values to it, adding its address to the list, and then returning. But since newEntry was a local variable, it'll fall off the stack as soon as that function returns. The next time you try and access that team in the list, that memory won't contain the added team any more, and you'll get either an access violation or just random data.

Instead, you need to allocate the entries dynamically (with 'new' - and don't forget to 'delete' when you're done) so that they're allocated in heap memory and not local stack memory. That way the allocated objects will persist outside of the function that created it.

Call Me Jimmy

satchmo wrote:

Does nobody use code tags any more?!

In the LeagueTable(Team*) constructor, the last line should be 'first = t;' and not 'first = &t;'. The way you have it at the moment, it's assigned the address of the pointer variable itself and not the address that's being pointed to. So when you try and access it, you're not accessing the address of the team, you're accessing the address of the pointer that points to the team. That's where your access violation is coming from.

Having a quick look through your code, I'd say you'll have problems with adding the address of local variables in your LeagueTable::add(Team*) function. You're declaring a local 'newEntry' variable, assigning a couple of values to it, adding its address to the list, and then returning. But since newEntry was a local variable, it'll fall off the stack as soon as that function returns. The next time you try and access that team in the list, that memory won't contain the added team any more, and you'll get either an access violation or just random data.

Instead, you need to allocate the entries dynamically (with 'new' - and don't forget to 'delete' when you're done) so that they're allocated in heap memory and not local stack memory. That way the allocated objects will persist outside of the function that created it.

Thanks very much for your help, I really appreciate it. BTW I tried using code tags but they didn't work <code> </code> ?? i did a test thread in the test forum and it wasn't working.

Yeah I don't have a deep understanding of how exactly c++ works (java here)
so I'll look into this later. Thanks again

satchmo

Yeah the forum tags are with square brackets! [tag] foo [/tag]

You're doing pretty well otherwise - the major difference between Java and C++ is the memory management, so I'm not surprised you're getting into a bit of trouble. All you need to do is read up on pointers, references, memory structure etc. It's not that hard to understand, but just takes a bit of getting used to. Have a read of these tutorials on pointers and dynamic memory - they're not too long and fairly easy to follow - and you should have a much better understanding of it.

Call Me Jimmy

I know you would have only glanced through my code so I'm wondering if you didn't notice that 'first' is actually a pointer in the leaguetable. So I am assigning it an address which I thought was the correct way to do it?

Did you think first was not a pointer?

satchmo

Oh I did notice alright. If you want to assign the address contained in one pointer (p1) to another pointer (p2), all you need to do is 'p2 = p1;'.

By saying 'first = &t;', you're assigning the address of the pointer variable t to the pointer first, when you actually want to assign the address that the pointer variable t contains, which you do by saying 'first = t;'. Geddit?!

Call Me Jimmy

satchmo wrote:

Oh I did notice alright. If you want to assign the address contained in one pointer (p1) to another pointer (p2), all you need to do is 'p2 = p1;'.

By saying 'first = &t;', you're assigning the address of the pointer variable t to the pointer first, when you actually want to assign the address that the pointer variable t contains, which you do by saying 'first = t;'. Geddit?!

hmmm well you are referring to 'first = &temp' yes? temp is of type struct entry which i defined. So i made an entry (not a pointer to an entry). An entry is a team and a pointer to the next entry

So what i did was, made a new entry, assigned it a team and a null pointer. I then wanted first to point to this new entry. Is it not correct to use 'address of' because the entry temp isn't a pointer?

If I remove the address of operator i get an error.

EDIT: Or are you basing this on the fact that I change my code to use the dynamic memory method you said? In which case temp would be a pointer.

EDIT - The Sequel: Hey it's working, I used the new operator etc. for dynamic memory etc. etc. and it's working. Thanks very much for taking the time out to help.

satchmo

Great! Glad you got it working, sounds like you're getting the hang of it.

The reason you were getting the original access violation was because of the memory you were using to store the variable. When you declare a variable inside a function, it's only in scope for the duration of that function. It gets allocated in memory on the stack (just like the memory for arguments etc), which all pops off once the function returns. So if you take the address of a variable inside that function and then try to access it once it goes out of scope - when the function returns - that memory won't be valid any more.

By using new to allocate memory for a variable (be it simple datatype, struct or class object), it gets allocated on the heap (system memory) and persists there until you explicitly delete it again. So you don't need to worry about it falling out of scope. The default stack is 1MB on Win32, which is another good reason for allocating larger objects dynamically on the heap, as your program will crash if you run out of stack memory.

Oh and don't forget to loop over the list in the destructor of LeagueTable and delete all the pointers to avoid a memory leak.

Call Me Jimmy

One thing I don't understand about this dynamic memory is why I have to do it in this situation:

/*==========================================================
Get():	
Given the team's name, this gets a pointer to the team.
===========================================================*/
Team* LeagueTable::get(string teamName){
	Team *t = new Team();
	list<Team>::iterator it;

	for(it = this->table->begin();it!= this->table->end();it++){
		if(*it == teamName){
			*t = *it;
			return t;
		}
	}
	return NULL;
}

All I wanted to do was return a pointer to an existing team in the list. What I don't understand is why i have to allocate memory for a whole new team instance just for a pointer to an existing team?

satchmo

I think life would be easier here (and in general) if you kept a list of Team pointers instead of a list of team objects. That way you can loop over pointers, pass pointers around, and it's completely unambiguous where you're creating and deleting the actual team objects.

The reason you had to create an object here (I presume it crashes if you don't) is because of the line '*t = *it;'. The first part, *t, is dereferencing the t pointer to get the actual object it points to, which you then copy the Team in the list into. If you executed the line '*t = *it;' without having t pointing to an object, it would try to dereference a Null pointer and assign a value to it, which will crash the program.

You should also be aware of the way that copying is done in this case. When copying one instantiated object into another, behind the scenes the compiler will create and use a 'copy constructor' to copy all of the values from one object into the other object. This is potentially a large copy (depending on the size of the Team class) which isn't good performance-wise. You want to be looping over and copying pointers (single 32bit values which fit in a register... fast), not the actual objects themselves.

There's a bigger problem with the code you have there: you're creating a new object every time you want to get one that's already in the List. You're copying the values of the team in the list into it alright, but you still end up with two separate objects in memory, both with the same values. This will definitely cause problems; imagine the following situation:

Team *a = league.get("Liverpool");
Team *b = league.get("Liverpool");
a->SetLeaguePoints(20);
float score = b->GetLeaguePoints(); //not 20!

In this case, both a and b should point to the same object, and changing one should change the other. But since your get function is creating a new object each time and returning that, you'll end up with pointers to two different objects which have nothing to do with each other.

By storing a List of pointers in the league table and working with them, you'll never have this problem.

Call Me Jimmy

Thanks very much. My next question was going to be about the fact that I was returning new objects. I just thought it was possible to declare a pointer and have it point to the exact instance that another pointer is pointing to. For example:

int x = 5;
int *ptr = x;

*ptr = 6;

and therefore x should be 6. I don't understand why I can't do that for my teams in the list. Create a pointer to them by using the pointer from the iterator and then change their values using this pointer.

satchmo

You're using the wrong syntax, it should be 'int *ptr = &x;'. The way you have it, you're saying ptr should point at the memory location 0x00000005 (the value of x), rather than the memory location of x (which is &x).

Call Me Jimmy

satchmo wrote:

You're using the wrong syntax, it should be 'int *ptr = &x;'. The way you have it, you're saying ptr should point at the memory location 0x00000005 (the value of x), rather than the memory location of x (which is &x).

dammit I knew that, I'm just confusing myself atm. Despairing about c++ in general despite your help. In java all this stuff is built in and glossed over, for example, would most c++ programmers just write their own list classes or use the STL? I have read tutorials but I'm still finding it hard. I think I would have found it easier if I didn't know java so well.

Mutant_Fruit

would most c++ programmers just write their own list classes or use the STL?

Most programmers would do both. STL for the standard stuff, then roll your own just like you would in java for the non-standard stuff. For those cases where you need performance and the standard classes don't cut it, you roll your own. Note: this situations aren't very common, most of the time the standard ones do just fine.

Call Me Jimmy

So is it not possible to go through an stl list which contains object instances and get references to those instances in order to change their data. I would have to create a list of pointers? The problem with this is I have to define operators for the pointers in order for the list to sort correctly.

satchmo

No that should be possible alright, it's just not the best way of going about it. Operator overloading is simple though, all you have to do is implement the < operator to compare the Team's name strings and return whichever is smaller alphabetically, then the STL sort will work.

Call Me Jimmy

So I'm back at this, trying to deepen my understanding of it. I have another related question.

If I were to create pointers to new objects and then add the values pointed to by the objects into a stl vector in a function like this:

void addToVector()
{

Object *o = new Object;
vector->push_back(*o);
}

would this mean that I can't delete the objects one by one from the vector?

satchmo

I'm not sure what you mean - are you worried about being able to delete a single object from the vector without deleting the entire vector?

When you do a push_back, it actually inserts into the vector a copy of the object you pass in. It makes this copy using the copy constructor (either one explicitly defined by you, or a compiler-generated one if you don't). So you need to be careful that the object's destructor doesn't delete any pointers that the object might contain, as any other copies hanging around won't know that their copy of that pointer isn't valid any more (a "dangling pointer"). Reference-counting smart pointers help with this, they keep a record of how many objects refer to them, and then only delete themselves when the reference count is zero.

This is yet another reason for keeping a vector of pointers and not a copy of the object itself!