Stuffed

Started by
2 comments, last by Zahlman 17 years, 11 months ago

int initialize_equation(equation* _equation)
{
	_equation = (equation*) malloc(sizeof(equation));

	if(_equation == 0)
		return 0;

	_equation->products = (compound*) malloc(sizeof(compound) * MAX_COMPOUNDS_PER_EXPRESSION);
	_equation->reactants = (compound*) malloc(sizeof(compound) * MAX_COMPOUNDS_PER_EXPRESSION);

	for(int i = 0; i < MAX_COMPOUNDS_PER_EXPRESSION; i++)
	{
		if(!initialize_compound(&_equation->products) || !initialize_compound(&_equation->reactants))
			return 0;
	}

	return 1;
}

/* Program Functions */

void end()
{
	printf("\n\nPress any key to continue...");
	getch();
}

int main(int argc, char* argv[])
{
	char* input = (char*) malloc(256);
	equation* expression = 0;

	atexit(end);

	initialize_equation(expression);

	if(!expression)
	{
		printf("Error initializing chemical equation.");
		exit(0);
	}


After I call initialize_equation(expression), expression becomes NULL. Any suggestions why, and how to fix it? Thanks.
A JPEG is worth a thousand and twenty four DWORD's. ;)
Note: Due to vacationing my website will not be updated till late-August. Of course I still have internet, but who wants to program during a vacation?
Advertisement
pass a pointer to a pointer to an expression

ie int initialize_equation(equation** _equation)

Cheers
Chris
CheersChris
wow thanks... now for the why part. Why would I need to use a pointer-to-pointer in order to make sure expression stays valid? would it help at all if i did:

equation expression;
init_equation(&expression); // Assuming the original version, not the Chris version.

Thanks again.
A JPEG is worth a thousand and twenty four DWORD's. ;)
Note: Due to vacationing my website will not be updated till late-August. Of course I still have internet, but who wants to program during a vacation?
By passing a double-pointer, you add another level of indirection. This allows the function to modify some data at a specific location that is "fed" by the caller, and as a consequence the caller "knows" about the change by looking at the same location.

It's actually exactly the same as if you wanted to work with a non-pointer:

void doSomethingWith(int x) {  x = 42;}int main() {  int foo = 23;  doSomethingWith(foo);  printf("%d\n", foo); /* foo is still 23! */}


This is because 'foo' is being copied onto the stack in order to be passed to the function, the function modifies the copy, and then it falls off the stack. Using a pointer, we get:

void doSomethingWith(int *x) {  *x = 42;}int main() {  int foo = 23;  doSomethingWith(&foo);  printf("%d\n", foo); /* foo is now 42 */}


This time, we copied a pointer value onto the stack, used the copied value in order to alias the same chunk of memory, and wrote the new value directly onto main()'s variable. (Which of course is also on the stack, but that's irrelevant here.)

That said: Don't do that here! The best thing (normally) would be to return an equation, letting it have null products and reactants if the initialization failed. You would allocate as much as possible on the stack, returning the equation by value. Trust me, things are much simpler this way.

That looks like this:

equation failed(equation toBeCleaned) {  if (toBeCleaned.products) free(toBeCleaned.products);  if (toBeCleaned.reactants) free(toBeCleaned.reactants);  return toBeCleaned;}// By the way, do not cast the return value of malloc() in C.equation initialize_equation() {  equation result;  result.products = malloc(sizeof(compound) * MAX_COMPOUNDS_PER_EXPRESSION);  result.reactants = malloc(sizeof(compound) * MAX_COMPOUNDS_PER_EXPRESSION);  if (!(result.products && result.reactants)) {     return failed(result);  }  for(int i = 0; i < MAX_COMPOUNDS_PER_EXPRESSION; i++) {    if(!initialize_compound(&result.products) ||        !initialize_compound(&result.reactants)) {      return failed(result);    }  }  return result;}


Please also note that you MUST clean up your dynamically allocated memory when something Goes Wrong(TM).

Alternatively, you might dynamically allocate the equation, and return a pointer to the new "object", or NULL if something failed.

equation* failed(equation* toBeCleaned) {  if (toBeCleaned) {    if (toBeCleaned->products) free(toBeCleaned->products);    if (toBeCleaned->reactants) free(toBeCleaned->reactants);  }  return toBeCleaned;}equation initialize_equation() {  equation* result = malloc(sizeof(equation));  if (!result) return failed(result);  result->products = malloc(sizeof(compound) * MAX_COMPOUNDS_PER_EXPRESSION);  result->reactants = malloc(sizeof(compound) * MAX_COMPOUNDS_PER_EXPRESSION);  if (!(result->products && result->reactants)) {     return failed(result);  }  for(int i = 0; i < MAX_COMPOUNDS_PER_EXPRESSION; i++) {    if(!initialize_compound(&result->products) ||        !initialize_compound(&result->reactants)) {      return failed(result);    }  }  return result;}


But honestly, this sort of thing is SO much easier in C++ it's not even funny. With far fewer lines of code, you can get the same effect WRT initializing things, automatic calls to your "initialize" and also "cleanup" methods (you DO clean up, yes?), no memory leaks, no obscure memory management bugs, and no arbitrary limitation on the number of items.

class Equation {  vector<Compound> products, reactants;  // If you REALLY want to bother:  Equation() {    products.resize(MAX_COMPOUNDS_PER_EXPRESSION);    reactants.resize(MAX_COMPOUNDS_PER_EXPRESSION);  }};// That just implemented all of initializeEquation and destroyEquation (or // whatever you call it), and now implementing main() is as simple as:int main() {  std::string input;  Equation expression;  // Although if you really needed to check for a failed memory allocation,  // you could do this:  //try {  //  Equation expression;  //} catch (std::bad_alloc& e) {  //  cout << "Error initializing chemical equation.");  //}  // But note that regardless, all the other allocated memory will be cleaned up  // and you never have to do any thinking about what memory needs to be freed/  // was already freed and shouldn't be re-freed.  atexit(end);}

This topic is closed to new replies.

Advertisement