Refactoring global to local. Should they be static or not?

Question

I'm refactoring "spaghetti code" C module to work in multitasking (RTOS) environment.

Now, there are very long functions and many unnecessary global variables.

When I try to replace global variables that exists only in one function with locals, I get into dilemma. Every global variable is behave like local "static" - e.g. keep its value even you exit and re-enter to the function.

For multitasking "static" local vars are worst from global. They make the functions non reentered.

There are a way to examine if the function is relay on preserving variable value re-entrancing without tracing all the logical flow?

Answer 1

Please give examples of what you call 'global' and 'local' variables

int global_c; // can be used by any other file with 'extern int global_c;'

static int static_c; // cannot be seen or used outside of this file.

int foo(...)
{
   int local_c; // cannot be seen or used outside of this function.
}

If you provide some code samples of what you have and what you changed we could better answer the question.

Answer 2

Short answer: no, there isn't any way to tell automatically whether the function will behave differently according to whether the declaration of a local variable is static or not. You just have to examine the logic of each function that uses globals in the original code.

However, if replacing a global variable with a static local-scope variable means the function is not re-entrant, then it wasn't re-entrant when it was a global, either. So I don't think that changing a global to a static local-scope variable will make your functions any less re-entrant than they were to start with.

Provided that the global really was used only in that scope (which the compiler/linker should confirm when you remove the global), the behaviour should be close to the same. There may or may not be issues over when things are initialized, I can't remember what the standard says: if static initialization occurs in C the same time it does in C++, when execution first reaches the declaration, then you might have changed a concurrency-safe function into a non-concurrency-safe one.

Working out whether a function is safe for re-entrancy also requires looking at the logic. Unless the standard says otherwise (I haven't checked), a function isn't automatically non-re-entrant just because it declares a static variable. But if it uses either a global or a static in any significant way, you can assume that it's non-re-entrant. If there isn't synchronization then assume it's also non-concurrency-safe.

Finally, good luck. Sounds like this code is a long way from where you want it to be...

Answer 3

If your compiler will warn you if a variable is used before initialized, make a suspected variable local without assigning it a value in its declaration.

Any variable that gives a warning cannot be made local without changing other code.

Answer 4

If I understand your question correctly, your concern is that global variables retain their value from one function call to the next. Obviously when you move to using a normal local variable that won't be the case. If you want to know whether or not it is safe to change them I don't think you have any option other than reading and understanding the code. Simply doing a full text search for the the name of the variable in question might be instructive.

If you want a quick and dirty solution that isn't completely safe, you can just change it and see what breaks. I recommend making sure you have a version you can roll back to in source control and setting up some unit tests in advance.

Answer 5

Changing global variables to static local variables will help a little, since the scope for modification has been reduced. However the concurrency issue still remains a problem and you have to work around it with locks around access to those static variables.

But what you want to be doing is pushing the definition of the variable into the highest scope it is used as a local, then pass it as an argument to anything that needs it. This obviously requires alot of work potentially (since it has a cascading effect). You can group similarly needed variables into "context" objects and then pass those around.

See the design pattern Encapsulate Context

Answer 6

If your global vars are truly used only in one function, you're losing nothing by making them into static locals since the fact that they were global anyway made the function that used them non-re-entrant. You gain a little by limiting the scope of the variable.

You should make that change to all globals that are used in only one function, then examine each static local variable to see if it can be made non-static (automatic).

The rule is: if the variable is used in the function before being set, then leave it static.

An example of a variable that can be made automatic local (you would put "int nplus4;" inside the function (you don't need to set it to zero since it's set before use and this should issue a warning if you actually use it before setting it, a useful check):

int nplus4 = 0;         // used only in add5
int add5 (int n) {
    nplus4 = n + 4;     // set
    return nplus4 + 1;  // use
}

The nplus4 var is set before being used. The following is an example that should be left static by putting "static int nextn = 0;" inside the function:

int nextn = 0;             // used only in getn
int getn (void) {
    int n = nextn++;       // use, then use, then set
    return n;
}

Note that it can get tricky, "nextn++" is not setting, it's using and setting since it's equivalent to "nextn = nextn + 1".

One other thing to watch out for: in an RTOS environment, stack space may be more limited than global memory so be careful moving big globals such as "char buffer[10000]" into the functions.