views:

153

answers:

5

Looking at this question that has just been asked: http://stackoverflow.com/questions/2231317/inconveniences-of-pointers-to-static-variables would doing something like this be considered bad practice, then?

char* strpart(char* string, int start, int count)
{
    char* strtemp; 
    int i = 0; int j = 0;
    int strL = strlen(string);

    if ( count == 0 )
    {
        count = strL;
    }

    strtemp = (char*) calloc((count + 1), sizeof(char));
    for ( i = start; i < (start+count); i++ )
    {
        strtemp[j] = string[i];
        j++;
    }
    return strtemp;
}

Sorry it's written quickly, but the basic principle is - when NOT using a static buffer inside a function is it bad practice to assign memory inside a function? I assume so because it wouldn't be freed, would it? Thought I ought to ask though.

+1  A: 

Well, it's dangerous. I'd try to avoid it when possible.

Your assumption is correct - the memory will not get released automatically.

The problem is that the return value here is memory allocated on the heap, the caller of your function has to remember to free. You're allocating memory here that will not (by you) get released. It's always a bad idea to put constraints on the user of your API.

Sometimes (rarely) this can't be avoided, so if you do this, make sure to document it very clearly.

Reed Copsey
Ok thanks, I didn't think it would get freed done like this so that's nice to know. Given the answers to the previous question it's fairly trivial to write a safe alternative anyway. Thanks again.
Ninefingers
+3  A: 

It's always OK practice to allocate memory dynamically inside a function PROVIDED you return a pointer to that memory to the outside world, so that something else can free it, or free it yourself within the function.

anon
+5  A: 

It's not bad pratice but it can easily create memory leaks (the callers have to remember to free the memory).

One thing I like to do is use a naming convention to indicate what functions can allocate. For example, I'd name that function:

char* strpart_alloc(char* string, int start, int count)
R Samuel Klatchko
One of the places where hungarian-style notation can really make a difference (if the dev even knows what the `_alloc` means..)
Dana the Sane
I like that idea. Ok so the correct method would then be to use char* ptr = strpart_alloc(...); do stuff with ptr; free(ptr)?
Ninefingers
@Ninefinger - yes, that is the correct way to call such a function.
R Samuel Klatchko
That's excellent, thanks!
Ninefingers
+1  A: 

It's common to do this. You just have to clearly note in your documentation "API" that it is the caller's responsibility to free the returned pointer when finished.

Nathan Kidd
+1  A: 

It is not a bad practice. The fact that the function returns a malloc-ed (or calloc-ed) memory becomes a part of its external specification. It becomes a the caller's responsibility to free it when it is no longer necessary.

It is inelegant though. It is inelegant since it 1) forces the use of dynamic memory, when the caller might prefer to avoid it, and 2) forces the use a specific kind of dynamic memory - the malloc-ed one, when the caller might prefer to use its own allocation mechanism.

AndreyT