Is it bad code design to use an out of scope static variable?

Question

I have a function that uses a static variable to count how many times it got called:

void func ()
{
    static int counter=0;
    counter++;

    /* some code */
}

The code in the function executes differently depending on the counter.

As it happens while expanding the program to have more features etc. the original implementation is insufficient and there are cases where counter needs to be modified to get the correct behavior.

This however turns out to be quite a challenge as the function in question is already in use all over the program, which means that changing things like the function parameters would break a lot of prexisitng code.

After thinking about this problem I came up with the following two solutions:

Solution 1:

int& func ()
{
    static int counter=0;
    counter++;

    /* some code */
    return counter//return an lvalue reference
}

int main() 
{
    /* some code */
    func()=2;//modify anonymous variable of type lvlaue reference, referencing counter
    /* some code */
    return 0;
}

Solution 2:

int* func ()
{
    static int counter=0;
    counter++;

    /* some code */
    return &counter//return a pointer
}

int main() 
{
    /* some code */
    *func()=2;//modify anonymous variable of type pointer, pointing to counter
    /* some code */
    return 0;
}

As far as I can tell it works, but to me it looks rather suspicious, especially since I haven't ever seen such practices.

I understand the code as follows in the first solution it returns counter by reference, so after func evaluates to an anonymous object in the expression in main(), which happens to be an lvalue reference to counter. That effectively lets main() modify a variable that's out of scope.

The second solution works analogously, but using a pointer instead of an lvalue reference. And since in both cases counter, as a static variable, has a lifetime that ends with the program the variable/memory address returned stays valid, even though it is accessed out of scope.

But is my interpretation correct? Is it undefined behavior after all and I just got lucky that it worked?

And if it is correct would it be considered bad code design given the circumstances?

Answer 1

Congratulations, you've figured out why hidden global state is bad.

Commiserations, you are going to have to change a whole bunch of code to fix this.

class Counter {
    int counter;
public:
    Counter(int counter = 0) : counter(counter) {}
    void func() { ++counter; /* some code */ }
}

int main() 
{
    Counter c;
    /* some code */
    c = Counter(2);
    /* some code */
    return 0;
}

Everywhere that was just calling func now needs a reference to a Counter.

I'm going to guess that one of the reasons you "need to modify" the counter is that you want to return to some previous state after having called func a bunch of times. Luckily you can easily achieve that with the Counter class, by copying it and operating on the copy.

I.e. instead of

void doStuffAndRevert() {
    int existing = func() - 1; // include the initial ++
    func();
    func() = existing; // reset to start
}

you can, by using pass-by-value to ensure you have a local copy:

void doStuffAndRevert(Counter c) {
    c.func();
    c.func();
    c.func();
}

Answer 2

If you think you want to be able to modify counter, then you create a function setCounter(value). Not caring at the moment how you do it. Having a function returning the address of the variable is an unacceptable hack.

And then it looks like the best implementation is something like static int counter_ = 0; outside any function. And both func() and setcounter() set that variable. It can only be modified by code inside the file, so that is as safe as possible. The trailing underscore to state clearly that you usually shouldn’t touch it.