Give me a few pointers on this code? haha.....ha...ha......*Mutter*

[HarryW]

Hello all you C++ fanatics. I've been (very) slowly learning C++ for a while now, and I thought I'd play about with classes, class constructors/destructors and linked lists. I'm not sure if I've done this the right way or not, but I'm sure you'll be able to advise me

This code will compile (was quite shocked cos it compiled almost forst time... almost) but I get errors, 'unresolved external symbol' at link-time. Any ideas why?

There's probably some big glaring error in the code, I hope I'm not gonna look like an idiot

#include <iostream>

using namespace std;

class CValue
{
private:
	long *value;
	CValue *next;
	CValue *prev;
	static int numVals;     //number of objects instantiated
	static CValue *first;	//first object in list
	static CValue *last;	//last object in list
	
public:
	//a few basic functions...
	int hexVal()
	{	cout << hex << *value << endl;
		return 0;
	}

	int decVal()
	{	cout << dec << *value << endl;
		return 0;
	}

	int octVal()
	{	cout << oct << *value << endl;
		return 0;
	}
	
	//some functions to access private member variables...
	int howMany()
	{	cout << "There are " << numVals
			 << " CValue objects in existence."
			 << endl;
		return numVals;
	}
	
	static CValue* getFirst()
	{	return first;
	}

	static CValue* getLast()
	{	return last;
	}

	CValue* getNext()
	{	return next;
	}

	CValue* getPrev()
	{	return prev;
	}

	//class constructor
	CValue(long x = 255)
	{	value = new long(x);
		if(numVals)
		{	first = this;
			last = this;
			next = this;
			prev = this;
		}
		else
		{	prev = last;
			next = first;
			last = this;
		}
		numVals++;
	}

	//class destructor
	~CValue()
	{	next->prev = prev;
		prev->next = next;
		numVals--;
		if(!numVals)
			first = last = prev = next = NULL;
		delete value;
	}
};

int main()
{	//make a few instances of CValue
	CValue firstVal;
	CValue secondVal(100);
	CValue arrayOfVals[5];
	CValue* current = CValue::getFirst();
	CValue* last = CValue::getLast();
	//print out a list of decimal values, one for each object
	if(current)
		do 
		{	current->decVal();
			current = current->getNext();
		} while(current && current != last);
	

	return 0;
}

Tadaaa.. any offers?

[parksie]

The destructor has to be virtual:

virtual ~CValue() {
    next->prev = prev;
    prev->next = next;
    numVals--;
    if(!numVals)
        first = last = prev = next = NULL;
    delete value;
}

[HarryW]

Ah right. Why's that? I don't know anything about virtual stuff yet.

[parksie]

I'm not completely sure why the destructor needs to be virtual. It's probably something to make sure that it can't be overloaded (?).

[V(ery) Basic]

I can answer most general C++ questions, including programming for MS Windows. Full understanding of the Standard Template Library, and very helpful for general application structure.

I know that that's a useless post, but I had to post that.

[parksie]

Cool...you went to ask me a question...hehehe.

It is a bit contrived, isn't it!

[HarryW]

In the book I'm learning from it's just covered class destructors and no mention is made of anything being virtual. Several examples are given without the use of virtual. Is it just something that you personally do whenever you write a class destructor or did something point to a need for a virtual function?

[parksie]

I don't know - I always use virtual because that's what the examples said, and what I've seen in most code anywhere. I have no idea why it is, I just use it.

I suppose it helps the compiler slightly.

[HarryW]

I tried making the destructor virtual today (took me a while I know) and I still get link errors

[parksie]

Can you post the whole build output, please?

[HarryW]

Certainly, sorry I should have done that before but I thought it would probably be a glaring error. It's something to do with the static data members of the class:

--------------------Configuration: messAround2 - Win32 Debug--------------------
Linking...
main.obj : error LNK2001: unresolved external symbol "private: static class CValue *  CValue::first" (?first@CValue@@0PAV1@A)
main.obj : error LNK2001: unresolved external symbol "private: static class CValue *  CValue::last" (?last@CValue@@0PAV1@A)
main.obj : error LNK2001: unresolved external symbol "private: static int  CValue::numVals" (?numVals@CValue@@0HA)
Debug/messAround2.exe : fatal error LNK1120: 3 unresolved externals

messAround2.exe - 4 error(s), 0 warning(s)

Know what that means? I mean, what do I do to fix it? I kind of assumed you'd try to compile it yourself to get the error messages.

[parksie]

Yeah...just tried it, same messages (at least I know it's on both, though).

Right, it compiles if you add this between the definition of your class and the definition of main:

CValue *CValue::first = 0;
CValue *CValue::last = 0;
int CValue::numVals = 0;

The problem is the linker doesn't know what to do with the static values, and they have to be initialised at file scope.
Also, I think you've got another problem - it linked okay, but prints "255" then crashes .

[HarryW]

Okay, thanks a lot, I'll do that

What's file scope mean? I guess this is obvious, I'm just wondering though.

[parksie]

It means the code goes into the file outside of any { } blocks.

[hitcgar]

Well it's been a while since my last C++ project and I don't have C++ on my present station but...a thought or 2

long *value;  // pointer to a long
...
int decVal()
{
 cout << dec << *value << endl; // output of a pointer to a pointer to long
 return 0;
}
...

You declared value as a pointer to a long - in your decVal
you print out a pointer to a pointer - is this what you really want to do? Why don't you just use a simple long?

parksie's note:

CValue *CValue::first = 0;
CValue *CValue::last = 0;
int CValue::numVals = 0;

works because you try to return a pointer to an instance of
your class before a value for 'first'etc. has been assigned.

In your constructor you initialize everthing based on
numVals which has not been initialized either.
That's why 'first' causes a crash - you only initialize in the if( numVals ) block.

Now I think the other problem is that you do a

current->getNext();

there is no next at this point so you try to assign a null to current.

Then there's

while(current && current != last);

current && current ???

&& is a logical AND that is always true. Did you want to do a bitwise AND? -> use just one &

Or did you mean to do
while(current && current->getNext() != last);
?


Anyway, have fun. C++ is a great language. Actually I miss her a little

[HarryW]

Okay... err... to answer some of your queries:

The reason why value is of type long* is because I am practising using DMA in class constructors/destructors. This is just a quick and simple practise for me because I'm learning about classes in C++. I think I explained some of this in my initial post.

On the subject of the crashes you mentioned (I haven't actually had any crashes as such, only link errors, and I still haven't tried the file scope static initialisation thing, been kinda busy) it says in my big book O' C that statics are initialised with the value 0, so the pointers start out as null. I check for this in my main code block before trying to print values for all the objects in my linked list. Whenever anything gets assigned a null pointer, that's intentional. Or I think it is.

Now this little gem:

while(current && current != last);

This is meant to be checking that there is a current object and the current object is not on the last object in the list. Perhaps I have my precedence wrong? In case there's confusion, this is what I would have written if I was being really careful:

while(current && (current != last));


So I don't think there are any errors there... feel free to point them out if you can see any. Bear in mind this in an educational exercise more than anything so I'm interested in anything you have to say.

[hitcgar]

just testing huh? Keep on codin'.

ALWAYS initialze pointers anyway.

As to the 'current && current!=last' line - it is better to
watch for precedence with the parenthesis as you say.

The != and == operators have precedence over the && op. so
actually the mistake is mine (last C++ project 1997!)
but still above comment is always safest.