CODEWITHSUNDEEP
cstruct
ant2009
ant2009

Reputation: 22696

return a static structure in a function

C89 gcc (GCC) 4.7.2

Hello,

I am maintaining someones software and I found this function that returns the address of a static structure. This should be ok as the static would indicate that it is a global so the address of the structure will be available until the program terminates.

DRIVER_API(driver_t*) driver_instance_get(void)
{
    static struct tag_driver driver = {
    /* Elements initialized here */
    };

    return &driver;
}

Used like this:

driver_t *driver = NULL;
driver = driver_instance_get();

The driver variable is used throughout the program until it terminates.

some questions:

  1. Is it good practice to do like this?
  2. Is there any difference to declaring it static outside the function at file level?
  3. Why not pass it a memory pool into the function and allocate memory to the structure so that the structure is declared on the heap?

Many thanks for any suggestions,

Upvotes: 3

Views: 2248

Answers (2)

AnT stands with Russia
AnT stands with Russia

Reputation: 320777

  1. Generally, no. It makes the function non-reentrable. It can be used with restraint in situations when the code author really knows what they are doing.

  2. Declaring it outside would pollute the file-level namespace with the struct object's name. Since direct access to the the object is not needed anywhere else, it makes more sense to declare it inside the function. There's no other difference.

  3. Allocate on the heap? Performance would suffer. Memory fragmentation would occur. And the caller will be burdened with the task of explicitly freeing the memory. Forcing the user to use dynamic memory when it can be avoided is generally not a good practice.

    A better idea for a reentrable implementation would be to pass a pointer to the destination struct from the outside. That way the caller has the full freedom of allocating the recipient memory in any way they see fit.

Of course, what you see here can simply be a C implementation of a singleton-like idiom (and most likely it is, judging by the function's name). This means that the function is supposed to return the same pointer every time, i.e. all callers are supposed to see and share the same struct object through the returned pointer. And, possibly, thy might even expect to modify the same object (assuming no concurrency). In that case what you see here is a function-wrapped implementation of a global variable. So, changing anything here in that case would actually defeat the purpose.

Upvotes: 3

Jonathan Leffler
Jonathan Leffler

Reputation: 755064

  1. As long as you realize that any code that modifies the pointer returned by the function is modifying the same variable as any other code that got the same pointer is referring to, it isn't a huge problem. That 'as long as' can be a fairly important issue, but it works. It usually isn't the best practice — for example, the C functions such as asctime() that return a pointer to a single static variable are not as easy to use as those that put their result into a user-provided variable — especially in threaded code (the function is not reentrant). However, in this context, it looks like you're achieving a Singleton Pattern; you probably only want one copy of 'the driver', so it looks reasonable to me — but we'd need a lot more information about the use cases before pontificating 'this is diabolically wrong'.

  2. There's not really much difference between a function static and a file static variable here. The difference is in the implementation code (a file static variable can be accessed by any code in the file; the function static variable can only be accessed in the one function) and not in the consumer code.

  3. 'Memory pool' is not a standard C concept. It would probably be better, in general, to pass in the structure to be initialized by the called function, but it depends on context. As it stands, for the purpose for which it appears to be designed, it is OK.

NB: The code would be better written as:

driver_t *driver = driver_instance_get();

The optimizer will probably optimize the code to that anyway, but there's no point in assigning NULL and then reassigning immediately.

Upvotes: 3

Related Questions