Archived

This topic is now archived and is closed to further replies.

nosajghoul

convoluted code

Recommended Posts

While studying c++ and directX, I come across some code that is just unbelieveably contorted and twisted. If the code were a person it would be one of those 8 year old russian contortionists that can put their feet on their head by bending backwards. Im just currious if anyone can make any simple, short, and utterly pointless convolutions of their own. Just for fun. Time for an example : (brace yourself for an array of function pointers that clears out pointer arrays, and the functions to do it. Dont even get me started on recursion.)
#include <stdio.h>

int (*pFnState)(float num);
char *text = NULL;

int GSstart(float num);
int GScredits(float num);
int GSclose(float num);

int GSstart(float num)
{
	if(!text) text = new char;

	sprintf(text, "num = %f", num);
	MessageBox(0, "GSstart() entered", text, 0); 

	if(text) {text = NULL; delete text;}
	pFnState = GScredits;
	return num + 1;
}

int GScredits(float num)
{
	if(!text) text = new char;

	sprintf(text, "num = %f", num);
	MessageBox(0, "GScredits() entered", text, 0); 

	if(text) {text = NULL; delete text;}
	pFnState = GSclose;
	return num + 1;
}

int GSclose(float num)
{
	PostQuitMessage(0);
	return 1;
}

void Fn()
{

/*Assigning the return value of a function that keeps changing what its poing to to a member of an array pointed at by x. This is the twisted part.*/

	const SIZE_OF_ARRAY = 5;
	int *x[SIZE_OF_ARRAY];

	pFnState = GSstart;

	for(int a = 0; a < SIZE_OF_ARRAY; a++)
	{
		xLink 

			
		

Share this post


Link to post
Share on other sites
quote:
if(text) {text = NULL; delete text;}

Ah, good. A conditional to ensure that the memory is leaked correctly.


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
quote:
Original post by Sneftel
quote:
if(text) {text = NULL; delete text;}

Ah, good. A conditional to ensure that the memory is leaked correctly.


How appropriate. You fight like a cow.


Am I missing something or did it just set a pointer to NULL and then delete it?? Have you gotten this code to actually run? I would think that would cause a run-time error.

Share this post


Link to post
Share on other sites
The correct way would be:
delete text; text = NULL;
Of course, I''m not even going into your misuse of new a few lines earlier... I''ll leave that to other posters.


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
Hey, Im self taught. Maybe if I werent a bus driver and lived in kansas Id know what you meant, (Im looking it up now). Tell me whats wrong with if(text) {text = NULL; delete text;} and why it makes memory leaks. Plus, post some convoluted code.

bored,
-Jason

Share this post


Link to post
Share on other sites
quote:
Original post by nosajghoul
Hey, Im self taught. Maybe if I werent a bus driver and lived in kansas Id know what you meant, (Im looking it up now). Tell me whats wrong with if(text) {text = NULL; delete text;} and why it makes memory leaks. Plus, post some convoluted code.

bored,
-Jason


Don't take offense. The pointer should hold the memory adress of, I imagine, an array of chars. So if you first set it to NULL (0) and then delete it, you are attempting to free memory at the memory adress 0.

edit:
And the main problem is that you no longer have anything holding the memory adress of the memory you allocated, so it is just taking up room, and you have no way to delete it.



[edited by - Rayno on October 8, 2003 2:39:11 AM]

Share this post


Link to post
Share on other sites
Oh, and BTW, the code there is using a "finite state machine". It''s an odd-looking beast, but useful. It''s the first time I''ve actually seen it done with function pointers directly, tho; virtual classes and switch statements are more common.


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
Well... it's like this.

delete text

will release the memory referred to by the pointer named 'text'. However, before that line, you set text = NULL; Thus, your code:
1) Tries to delete memory it doesn't own (the memory at NULL)
1) Calls delete on a null pointer.
2) Fails to delete the memory that text originally pointed to.

edit: ok, ok... delete null is a no-op. so it's not really trying to delete memory it doesn't own... just doing nothing.

[edited by - tibre on October 8, 2003 2:43:29 AM]

Share this post


Link to post
Share on other sites
What they mean is that you are making text point at NULL instead of the previously allocated memory. Then, you are trying to delete a NULL pointer. This is bad for 2 reasons (maybe only one). First, you're trying to delete memory that you didn't allocate. Second, you're trying to delete a NULL pointer. This normally causes a runtime error (such as a crash).

The correct way to delete memory would be

if (text) { delete text; text = NULL; }

And that will not leak memory.

Now, about the misuse of the new operator...

There's really no point in allocating just 1 char so text shouldn't be a char *. it should just be of type char. Then you don't have to worry about deleting it either.

I can understand that you used new to do that since you are trying to make horribly convoluted code. But at least fix the delete problems.

I'm learning, just like the best of us...

Ok, now assume a spherical cow...

Apparently it took me 3 minutes to write that, so people got to you before me

[edited by - Soulkeeper on October 8, 2003 2:44:03 AM]

Share this post


Link to post
Share on other sites
Crap, now I have questions.

So if I NULL first, delete isnt necessary, is it? Null makes it point to memory address 0 (zero), while delete returns memory to the heap. Hence my thinking: make it nothing, then free it.

Rayno : no offence taken, actually, thanks.

Sneftel : I fight like a cow on hell difficulty with 8 players going after a necro.

Soulkeeper : (This normally causes a runtime error (such as a crash). ) ran fine, closed fine, no errors. Weird huh.




Share this post


Link to post
Share on other sites
quote:
Original post by nosajghoul
Crap, now I have questions.

So if I NULL first, delete isnt necessary, is it? Null makes it point to memory address 0 (zero), while delete returns memory to the heap. Hence my thinking: make it nothing, then free it.

Rayno : no offence taken, actually, thanks.

Sneftel : I fight like a cow on hell difficulty with 8 players going after a necro.

Soulkeeper : (This normally causes a runtime error (such as a crash). ) ran fine, closed fine, no errors. Weird huh.





Well not quite.

Lets say you have some memory allocated at memory adress 55. So your pointer literly holds the value 55. If you set your pointer to 0, then it doesn''t free the block of memory you allocated, it just makes you pointer, point to the address 0.

The reason pointers are set to NULL after is so if you accidentally accesss the memory pointed to by your pointer, it is safer trying to read/write to 0, than let''s say 55.

Share this post


Link to post
Share on other sites
to clarify the earlier posts, regarding dynamic allocation and deallocation.

since you wanted to write a string to your allocated memory you need to allocate an array of characters

if(!text) text = new char[someArbitraryNumberOfCharacters];

and once you are done with it

delete[] text;
text = NULL;

its not entirely neccesary to call that version of delete[] on an array of characters,you could just use the normal delete, but if you have an array of classes then delete[] will call the destructors of those classes correctly.

And as allready stated

char *text = NULL;
delete[] text;
text = NULL;

is a safe operation, it won't attempt to delete system memory.


EDIT: good practice to set back to NULL

[edited by - gleno on October 8, 2003 3:07:49 AM]

Share this post


Link to post
Share on other sites
If you don''t delete it, the memory never gets returned to the heap. Thus, your program has a memory leak.

Technically, you never have to set it to NULL. Some people just do that for cleanliness/safe coding purposes.

The super safe way is:

if (text) {
delete text;
text = NULL;
}


Some people do:

if (text)
delete text;


And apparently you can also do:

delete text;

Since delete on a null is apparently no-op.

Share this post


Link to post
Share on other sites
quote:
Original post by nosajghoul
Sneftel : I fight like a cow on hell difficulty with 8 players going after a necro.

Actually, that''s my signature. Still, I''m glad to hear it. The world needs more badass cows.


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
quote:
Original post by Tibre
The super safe way is:

if (text) {
delete text;
text = NULL;
}




Nope, the if(text) call is completely unnecessary.

delete [] text; // (or delete text, depending if text is an array or not)
text = NULL;

is just fine.

Share this post


Link to post
Share on other sites
Of course, one could ask why you aren''t using std::string, and std::stringstream, since you''re using c++

#include <string>
#include <sstream>
int GSstart(float num)
{
std::ostringstream textStream;
//use an ostringstream like cout

textStream << "num = " << num;

//use .str() to get a std::string out of a std::stringstream

std::string text = textStream.str();

//use .c_str() to get a const char* out of a std::string

MessageBox(0, "GSstart() entered", text.c_str(), 0);
pFnState = GScredits;
return num + 1;
}

Share this post


Link to post
Share on other sites
If youre getting bored of function pointers allready try it using poymorphism.

like this.



class State;
class StartState;
class CreditsState;
class CloseState;

State *currentState = NULL;
StartState *startState = NULL;
CreditsState *creditsState = NULL;
CloseState *closeState = NULL;

class State{
public:
virtual int run(float value) = 0;
}

class StartState : public State{
public:
int run(float value)
{
//add stuff

currentState = creditsState;
}
}

class CreditsState : public State{
public:
int run(float value)
{
//add stuff

currentState = closeState;
}
}

class CloseState : public State{
public:
int run(float value)
{
//add stuff

}
}


startState = new StartState;
creditsState = new CreditsState;
closeState = new CloseState;

currentState = startState;

//do whatever else you do, calling the current state like

currentState->run(num);



EDIT: some big mistakes, yes definitely convoluted.

[edited by - gleno on October 8, 2003 3:31:35 AM]

Share this post


Link to post
Share on other sites