[ossobuco]

> No, not in the face of any sort of concurrency it doesn't...

I fail to see how does that relate to the article in question.

The author provided an example for which counter() works well, he didn't claim that the same "pattern" would be good for 100% of the use cases.

[OskarS]

There are many reasons why global variables are bad, but extremely high on the list (if not first) is concurrency issues. The fact that the author think it's acceptable in any C or C++ code base to put in code like:

    static int prv_counter;
    int counter() { return ++prv_counter; }

Is insanity. Like, this is programmer malpractice. This function can only ever be called from one thread (not to mention: using `int` instead of `int64_t` is also a trivial mistake, this easily overflows). This is the kind of thing that enters a code base, works fine for long enough that everyone forgets about it, then causes horrible security issues and crashes. The idea of saying this is "good use" of a global variable is... this person should not be giving advice on good coding.

If you want to do this (and you shouldn't, because global state is bad for 14 other reasons), at the very least, make it thread safe and not trivially overflowing:

    static std::atomic<int64_t> prv_counter { 0 };
    int64_t counter() { return 1 + prv_counter.fetch_add(1, std::memory_order_relaxed); }

(the 1+ is because the original author used pre-increment instead of post-increment)

Like, this is not awesome, and you shouldn't do it, but it's at least not a total disaster.

EDIT: actually, this is also bad, because the `prv_counter` is not really private at all. The better way to do that would be:

    int64_t counter() { 
        static std::atomic<int64_t> prv_counter { 0 };
        return 1 + prv_counter.fetch_add(1, std::memory_order_relaxed);
    }

Three different serious issues in two lines of code, fun!

[ossobuco]

Except the author's example is single threaded, so for that specific case your implementation of counter is needlessly complex and would actually be confusing.

The point is that there is no solution that works for all use cases. If you always attempt to write fully generalized code like that you'll end up with tons of unnecessary complexity. Solve the problem at hand, not some hypothetical.

The author even specifies a rule that covers your case:

"If you're using threads, global and static variables should be thread local. If they are not then the discussion becomes about sharing data across threads and synchronization, which is a different topic."

[OskarS]

I'm sorry, but no. This is one of the primary reasons why global variables is bad, because of concurrency issues. If you add this to a code base, are you guaranteeing that at no point in the next decade, NOBODY is going to add a second thread? What if this is a library? What if some other library you're using runs a thread?

Doing this is planting a time-bomb in your code-base. It's going to lay dormant for a long time, and then explode, and be impossible to debug. This is why mutating global variables is `unsafe` in Rust, which is the right call. There is literally no C or C++ code base where I would allow this to pass code review, regardless of how single-threaded the author thinks it is.

> needlessly complex

It looks more complex than it is because I set the memory order to relaxed as an optimization. You could also write this:

    int64_t counter() { 
        static std::atomic<int64_t> prv_counter { 0 };
        return ++prv_counter;
    }

It's really not hard.

If this was a junior making this mistake, I would sit down and talk to them and patiently and explain the issues. If you're writing a blog post on the internet saying "this is a good way to write code, do it like this!", I'm sorry, but I'm going to be very harsh. We haven't even discussed using `int` and not properly scoping the static, also rookie mistakes. Someone is going to read this article, and think this kind of code is ok. It is not.

> "If you're using threads, global and static variables should be thread local. If they are not then the discussion becomes about sharing data across threads and synchronization, which is a different topic."

This is nonsense. First off all, a counter like this wouldn't work if it was a thread_local, because then you'd get duplicate values. If duplicate values are ok, why is it global state at all? The only reason you'd make it global state is to produce unique values for that process run. And the general sentiment is like saying "this is how you drive drunk safely. We're not discussing keeping control of your car and crashing into a tree, that's a different topic".

This IS why global variables are bad! Not the only reason, but a pretty darn big one!

[ossobuco]

Not all the work we do is as part of large teams or on multimillion loc codebases.

In your context what you say is definitely the correct take, but I stand by my take that there are relatively simple programs written by individuals/small teams that don't need to be complicated by corporate scale concerns.