return char pointer which is malloc'ed in function

[Khookie]

Hi everyone

I'm not sure whether this is good practice or not, so I thought I'll
ask.

I've got a function that returns a char pointer, which is malloc'ed in
the function itself, as per below code. It's basically a singly
linked list of strings that are concatenated into a big string.

My question is - since you have to know to free it yourself (usage
below), is this considered bad practice? Or should the user of the
function know?

Function:
char* string_list_concat(string_list *list) {
char* result = malloc(list->length + 1);
char* result_ptr = result;

// Create string
string_node *node = list->bol;
while (node != list->eol) {
char* string_node_ptr = node->string;
while(*string_node_ptr != '\0') {
*result_ptr = *string_node_ptr;
string_node_ptr++;
result_ptr++;
}
node = node->next;
}
*result_ptr = '\0';
return result;
}

Usage:
char* result = string_list_concat(list);
printf("Output is: %s\n", result);
free(result);

Cheers

Chris

 

[Chris Dollin]

I've got a function that returns a char pointer, which is malloc'ed in
the function itself, as per below code. It's basically a singly
linked list of strings that are concatenated into a big string.

My question is - since you have to know to free it yourself (usage
below), is this considered bad practice? Or should the user of the
function know?

(answer below)

Function:
char* string_list_concat(string_list *list) {
char* result = malloc(list->length + 1);

That can't be right: I think you have a missing multiplication by
`sizeof *result`.

To answer your question:

(a) the documentation of the function should make it clear who's
responsible for the storage it allocates.

(b) don't make it hard to keep track of responsibilities. Sometimes
you'll want to introduce idioms and functions to help you.

(c) respect the responsibilities given to you.

For (b), there are various things you can do to reduce the load, eg:

* write functions like

void with_concat_do( string_list *list, void (*it)( string_list * ) )
{
string_list * list = concatenated list;
it( list );
free( list );
}

so that the user /can't/ forget to call `free` (unless an `exit` or
`longjmp` happens, in which case you're pretty much skewered anyway).

* pass in an additional store-managing object which is already the
caller's responsibility and use that to allocate store.

 

[Joachim Schmitz]

(answer below)


That can't be right: I think you have a missing multiplication by
`sizeof *result`.

Which here is 1 anyway, sizeof(char)...

Bye, Jojo

 

[Chris Dollin]

Which here is 1 anyway, sizeof(char)...

Colour me idiotic: I kept seeing `result` there as `char **`. Thanks, Jojo.
Apologies to Khookie (and everyone else) for inflicting my Stupidity Of
The Day [1] on them.

[1] Please let it be the only one.

 

[regis]

Which here is 1 anyway, sizeof(char)...

one would have expected that list->length
is the number of nodes of the list, instead of
the sum of strlen() applied to all node->string of the list...