Where should I deallocate memory within functions?

Question

I'm writing a shell in C. While I don't expect many other people to use it, I'd like to practice writing maintainable and well-organized code. I've noticed the following pattern in a number of my functions, so before it solidifies, I'd like it to be fully vetted.

As an example, consider the following function:

int foo(int param...) {
  // declare variables
  struct bar *a, *b, *c;

  // do some work
  a = bar_creator();
  b = bar_modifier(a);
  c = bar_modifier(b);

  // cleanup
  free(a);
  free(b);
  free(c);

  return 1;
}

Things to note:

• three phases: declaration, initiation/modification, cleanup

• newly allocated structures are often returned from functions as modified copies of other objects

• a huge number of objects are not needed, so memory usage is not an issue

As it stands, the three sections have been relatively distinct. This allows me to match up the first and last sections and ensure everything is accounted for. Now I wonder if a better style might be to deallocate something as soon as it is not needed. A motivation for this might be to minimize the context within which a code section makes sense.

What is your approach to deallocation of resources? What are the advantages of the given strategy?

edit

To clear up any confusion as to the behavior of functions:

/**
* returns a newly created bar
*/
struct bar *bar_creator();

/**
* takes a bar, and returns a _new_ copy of it that may have been modified.
* the original is not modified.
*/
struct bar *bar_modifier(struct bar *param);

Answer 1

Why are you freeing it 3 times?

If bar_creator() is the only function that allocates memory dynamically you only need to free one of the pointers that point to that area of memory.

Answer 2

Personally, my preference is to free objects directly after I'm done using them, and only allocate directly before I need them. This forces me to understand what memory my program is actually using. Another benefit of this technique is that it reduces total memory consumption if you allocate additional memory after you free memory in the method.

Answer 3

I tend to group frees at the end unless I am reusing a variable and need to free it first. This way it's clearer what exactly needs to be destroyed, which is helpful if you are considering an early return or if the function is a bit more complex. Often your function will have a few different control flows and you want to be sure they all hit the clean up at the end, which is easier to see when the cleanup code is at the end.

Answer 4

I usually favor the smallest scope as possible, thus I create object as late as possible, and I release (free) them as early as possible.

I will tend to have:

char * foo;
/* some work */
{
foo = create();
/* use foo */
destroy(foo);
}
/* some other work */
{
foo = create();
/* use foo */
destroy(foo);
}

Even if I could have reused the memory, I prefer to alloc it twice and release it twice. Most of the time the performance hit of this technique is very little, as most of the time the two objects are different anyway, and if it's a problem, I tend to optimize this very lately in the dev process.

Now if you have 2 objects with the same scope (or three as your example), it's the same thing:

{
foo1 = create();
foo2 = create();
foo3 = create();
/* do something */
destroy(foo1);
destroy(foo4);
destroy(foo3);
}

But this particular layout is only relevant when the three objects have the same scope.

I tend to avoid this kind of layout:

{
foo1 = create();
{
    foo2 = create();
    /* use foo2 */
}
destroy(foo1);
/* use foo2 again */
destroy(foo2);
}

As I consider this broken.

Of course the {} are only here for the example, but you can also use them in the actual code, or vim folds or anything that denote scope.

When I need a larger scope (eg global or shared), I use reference count and a retain release mechanism (replace create with retain and destroy with release), and this has always ensured me a nice and simple memory management.

Answer 5

When you have finished with it!

Do not let cheap memory prices promote lazy programming.

Answer 6

there are two different situations to consider:

(1) an object is created in the local scope and it is not needed outside this local scope. in this case you could allocate storage with calloc alloca() or with a RAII approach. Using calloc alloca() has the big advantage that you don't have to care about calling free() because the allocated memory is automatically freed when the local scope is left.

(2) an object is created in the local scope and it is needed outside this local scope. In this case there is no general advice. I would free the memory when the object is no longer needed.

EDITED: use alloca() instead of calloc()

Answer 7

Let the compiler clean the stack for you?

int foo(int param...) {
  // declare variables
  struct bar a, b, c;

  // do some work
  bar_creator(/*retvalue*/&a);
  bar_modifier(a,/*retvalue*/&b);
  bar_modifier(b,/*retvalue*/&c);

  return 1;
}

Answer 8

1. Usually dynamically allocated memory has a long lifetime (longer than a function call) so it is meaningless to talk about where within a function it is deallocated.

2. If memory is only needed for within the scope of a function, depending on the language it should be statically allocated if appropriate on the stack (declared as a local variable in the function, it will be allocated when the function is called and freed when the function exits, as shown in an example by another poster).

3. As far as naming is concerned, only functions that allocate memory and return it need to be specially named. Anything else don't bother saying "modfiier" - use that letterspace for describing what the function does. I.e. by default, assume that it is not allocating memory unless specifically named so (i.e. createX, allocX, etc.).

4. In languages or situations (i.e. to provide consistency with code elsewhere in the program) where static alllocation is not appropriate, then mimic the stack allocation pattern by allocating at the beginning of the function call, and freeing at the end.

5. For clarity, if your function simply modifies the object, don't use a function at all. Use a procedure. This makes it absolutely clear that no new memory is being allocated. In other words, eliminate your pointers b and c - they are unnecessary. They can modify what a is pointing to without returning a value.

6. From the looks of your code, either you are freeing already freed memory, or bar_modifier is misleading named in that it is not simply modifying the memory pointed to by a, but creating brand new dynamically allocated memory. In this case, they shouldn't be named bar_modifier but create_SomethingElse.

Answer 9

For complicated code I would use structure charts to show the way the subroutines work together and then for allocation/deallocation I try to make these occur at roughly the same level in the charts for a given object.

In your case, I might be tempted to define a new function called bar_destroyer, call this 3 times at the end of function foo, and do the free() in there.

Answer 10

You need to be careful about what happens when the memory allocation fails. Because C doesn't have support for exceptions, I use goto to manage unwinding dynamic state on error. Here's a trivial manipulation of your original function, demonstrating the technique:

int foo(int param...) {
  // declare variables
  struct bar *a, *b, *c;

  // do some work
  a = bar_creator();
  if(a == (struct bar *) 0)
    goto err0;
  b = bar_modifier(a);
  if(b == (struct bar *) 0)
    goto err1;
  c = bar_modifier(b);
  if(c == (struct bar *) 0)
    goto err2;

  // cleanup
  free(a);
  free(b);
  free(c);

  return 1;

err2:
  free(b);
err1:
  free(a);
err0:
  return -1;
}

When using this technique, I always want to have a return statement preceding the error labels, to visually distinguish the normal return case from the error case. Now, this presumes that you use a wind / unwind paradigm for your dynamically allocated memory... What you're doing looks more sequential, so I'd probably have something closer to the following:

  a = bar_creator();
  if(a == (struct bar *) 0)
    goto err0;
  /* work with a */
  b = bar_modifier(a);
  free(a);
  if(b == (struct bar *) 0)
    goto err0;
  /* work with b */
  c = bar_modifier(b);
  free(b);
  if(c == (struct bar *) 0)
    goto err0;
  /* work with c */
  free(c);

  return 1;
err0:
  return -1;

Answer 11

Consider using a different pattern. Allocate variables on the stack if it is reasonable to do so (using declarations, not alloca). Consider making your bar_creator a bar_initialiser which takes a struct bar *.

Then you can make your bar_modifier look like

void bar_modifier(const struct bar * source, struct bar *dest);

Then you don't need to worry so much about memory allocation.

In general it is nicer in C to have the caller allocate memory, not the callee - hence why strcpy is a "nicer" function, in my opinion, than strdup.