tags:

views:

489

answers:

10

In C, which is the better practice when it comes to freeing memory returned from functions:

  • Provide a "destructor" function that encapsulates the call to free().
  • Require users to free() the returned pointer themselves.

For example, to open and close a file we do:

FILE* f = fopen("blah", "w");
fclose(f);

Is this preferable to:

FILE* f = fopen("blah", "w");
fclose(f);
free(f);

Warning: Don't call free() on a FILE pointer. I only use it a hypothetical implementation here.

And what about cases where local variables are pointed to the returned memory? Is free() harmful here? (or perhaps this should never be done)

FILE f = &fopen("blah", "w");
fclose(&f);
+4  A: 

You can't free FILE* it wasn't allocated by malloc.
Since you weren't responsible for allocating it - you shouldn't free it.

Martin Beckett
I'm assuming that the memory is malloc'd inside fopen.
fexii
Sorry malloc/free (new/delete in C++)The same rules apply, if fopen() malloced it then fclose() is responsible for it
Martin Beckett
Have to agree with MGB on this one.
Software Monkey
@fexii, classically fopen() returned a pointer to an element of a statically allocated array of FILE objects. There was no call to malloc(). This is why the standard says that fopen() will support at least FOPEN_MAX concurrent open files.
RBerteig
A: 

fopen returns a pointer to C library internally managed FILE structure, which is released (not necessarily freed) by call to fclose.

Nikolai N Fetissov
You're addressing the example, rather than the question.
Douglas Leeder
What is wrong with that?
Nikolai N Fetissov
+2  A: 

FILE* f is a pointer to a FILE object that is used to identify the stream on all further operations involving file. You shouldn't use free(f) as the memory is not allocated by malloc().

fclose is just enough to close the file associated with the stream.

About your question on providing destructor kind of function to free the memory: I feel it is appropriate to provide a destructor kind of function if the function does more than freeing the memory.

wrapperFree(Pointer* p)
{
 //do some additional work [ other cleanup operations ]
 free(p);
}

Also, for WrapperFree associated WrapperAllocate need to be provided.

Otherwise, I think the thumbrule would be for every malloc() call free() explicitely.

aJ
You're addressing the example, rather than the question.
Douglas Leeder
+9  A: 

You should never free a file - fclose handles releasing the resources properly. In general, only free pointers that were allocated directly by malloc. Most other pointers will have their own resource-cleanup functions.

That being said, as to your initial question:

I find that providing a destructor function is usually better practice, for three reasons.

1) There are many cases where free is inappropriate, and this might not be obvious to your end user. FILE* is a good case of that - you shouldn't call free(f); above...

2) If you're using this in a DLL, depending on the runtimes, having the free functionality encapsulated can work around many, many subtle bugs from mixing runtimes, especially on the Windows platform. Trying to use a DLL compiled in VS2005 from VS2008 can cause problems if you happen to free memory in one platform's code which was allocated in another. Having a "wrapper" function to handle the memory management works around that significant issue.

3) Many of the C API functions work this way - such as FILE* using fopen/fclose. This will not be surprising to a user of your library.

Reed Copsey
Thanks, these reasons make it clear that providing a "destructor" function is preferable.
fexii
+13  A: 

The best option for allocating and freeing memory is to do it symmetrically. i.e. If the caller allocates memory, let the caller free it. If your API allocs memory (callee), then your API should free it.

Example of caller alloc/free:

int * mymem = (int *)malloc(20 * sizeof(int));
...
a_func_to_call(mymem);
...
free(mymem);

Example of callee alloc/free:

FILE* f = fopen("blah", "w"); // allocs a FILE struct
fclose(f); // The implementation of fclose() will do what's necessary to 
           // free resources and if it chooses to deallocate any memory
           // previously allocated
Adam Markowitz
+1 I think this is a good rule of thumb for API design.
Doug
+1, because you haven't waste your/our time to misunderstand his question, like the other did.
quinmars
A: 

I guess you meant malloc() and not fopen() . malloc() allocates memory and returns the address of the allocated memory. This has to be released using free() call.

Naveen
A: 

As mgb says, there's no need to free() a FILE *. But in typical use there are library calls and your own code that will allocate memory and make you responsible. Here are some loose guidelines:

  • Make a "destructor" function when something needs to happen apart from freeing the memory. The FILE * stuff is a fine example: it must deal with the file itself as well as dealing with the memory.
  • Use bare free() when it's just a chunk of memory. Anything else is overkill.
dwc
A: 

There's no best practice without context...

But I'd say the principle of information-hiding implies that details of allocation and deallocation of memory used by a function should generally be encapsulated within the function.

The open/allocate function should return an opaque handle of some kind that is closed/deallocated by a close function.

FILE* can be thought of as such an opaque handle.

Joe
A: 

As previously mentioned, wrapping your "housekeeping" code inside corresponding functions (i.e. constructor/destructor) is generally good practice, in particular it may also be useful when dealing with exception handling, or even when specifying your own exit/signal handlers:

In such cases, having a destructor helper is very useful, because you can simply register them with functions such as atexit() so that they're automatically called upon program termination, so that global state can be properly cleaned up.

none
A: 

I not only like destructor functions but like Dave Hanson's convention which takes the address of the pointer and zeroes out the pointer as the memory is freed:

Thing_T *Thing_new(void);
void Thing_dispose(Thing_T **p);   // free *p and set *p = NULL
Norman Ramsey